EDM-4977: fix inventory plugin to use OIDC password grant instead of basic auth - #62
Conversation
…ugin The inventory plugin was sending HTTP Basic Auth when username/password were provided, but the RHEM server only accepts Bearer tokens via OIDC. This replaces the broken Basic Auth path with an OIDC password grant flow that mirrors the flightctl CLI behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughUsername/password authentication now uses OIDC discovery and a password-grant token request. Connection setup stores the bearer token and clears submitted credentials. Fleet processing accepts model objects and dictionaries. Inventory retrieval calls now pass configured request timeouts. ChangesFlight Control inventory integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InventoryPlugin
participant OIDCDiscovery
participant TokenEndpoint
InventoryPlugin->>OIDCDiscovery: Discover token endpoint
OIDCDiscovery-->>InventoryPlugin: Return endpoint metadata
InventoryPlugin->>TokenEndpoint: Submit password-grant credentials
TokenEndpoint-->>InventoryPlugin: Return bearer token
InventoryPlugin->>InventoryPlugin: Store token and clear credentials
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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 `@tests/unit/plugins/inventory/test_flightctl.py`:
- Around line 550-557: Update test_no_basic_auth_ever_produced to assert the
exact return value of _build_auth_headers(config) directly, expecting None for a
configuration without an access token, username, or password-based Basic Auth
output; remove the conditional headers check.
- Line 3: Remove the unused PropertyMock symbol from the unittest.mock import
statement, keeping only the mock utilities used by the tests so the F401 lint
violation is resolved.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48e83519-0145-4248-8698-0d171e6d0bb5
📒 Files selected for processing (2)
plugins/inventory/flightctl.pytests/unit/plugins/inventory/test_flightctl.py
Resolve import conflict in test_flightctl.py: combine OIDC test imports (json, PropertyMock, _build_auth_headers, ValidationException) with env declaration test imports (ClassVar, yaml, DOCUMENTATION) from EDM-4975. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…covery - Replace urllib.request.urlopen with ansible.module_utils.urls.open_url to pass Ansible sanity tests (replace-urlopen check) - Fetch OIDC provider config from /api/v1/auth/config instead of hardcoding the discovery path and client_id - Fix pep8 line-length violation in DOCUMENTATION block - Remove unused PropertyMock import and unnecessary lambda (pylint) - Make test_no_basic_auth_ever_produced assertion unconditional Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/plugins/inventory/test_flightctl.py (3)
557-563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test_no_basic_auth_ever_producednow duplicatestest_no_headers_when_no_tokenexactly.After the assertion fix, both tests build the same
configand assert the sameassertIsNone(headers). The two names promise two different guarantees, but the bodies are identical, so the suite reports two passes for one behavior. A reader who later changes_build_auth_headersgets no extra signal from the second test.Give the second test a distinct scenario. Assert that a config carrying both a token and credentials still yields only a Bearer header, which is the anti-Basic-Auth guarantee the name claims.
♻️ Proposed distinct scenario
def test_no_basic_auth_ever_produced(self): config = MagicMock() - config.access_token = None + config.access_token = "my-jwt-token" config.username = "admin" - config.password = "secret" + config.password = "placeholder-password" headers = _build_auth_headers(config) - self.assertIsNone(headers) + self.assertEqual(headers, {"Authorization": "Bearer my-jwt-token"}) + self.assertNotIn("Basic", headers["Authorization"])As per path instructions: "Each test verifies one behavior".
🤖 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/inventory/test_flightctl.py` around lines 557 - 563, Update test_no_basic_auth_ever_produced to use a configuration containing both a non-null access token and username/password credentials, then assert that _build_auth_headers returns only the expected Bearer header and no Basic-Auth credentials. Keep test_no_headers_when_no_token unchanged so each test covers a distinct behavior.Source: Path instructions
923-953: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese eight tests restate
test_all_connection_options_have_env_declarations.The table-driven test at lines 910-921 already asserts, for every option in
EXPECTED_ENV_VARS, that the option exists, thatenvis a list, and that it contains the expected variable name. Each of these eight methods asserts a strict subset of that, and each is weaker because it pins only index 0.Adding a ninth option therefore requires two edits in this file instead of one, and the table stays the real source of truth either way. Delete the eight per-option methods and keep the table-driven test.
🤖 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/inventory/test_flightctl.py` around lines 923 - 953, Remove the eight per-option test methods from the test class: test_token_env_declaration, test_host_env_declaration, test_username_env_declaration, test_password_env_declaration, test_organization_env_declaration, test_config_file_env_declaration, test_ca_path_env_declaration, and test_verify_ssl_env_declaration. Retain test_all_connection_options_have_env_declarations as the single table-driven source of truth for environment declarations.
955-960: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
test_env_vars_match_module_utils_conventioncannot fail.The loop reads
expected_envfromEXPECTED_ENV_VARS, which is a literal defined ten lines above in this same file, and then asserts that the literal starts withFLIGHTCTL_. No plugin code participates. The test passes even ifDOCUMENTATIONdeclares noenvblocks at all, or declaresAWS_TOKEN.The docstring claims the test enforces the convention "used in module_utils/core.py", but nothing imports or reads
module_utils/core.py.Assert against the values parsed from
DOCUMENTATIONinstead, so the plugin is the subject under test.💚 Proposed assertion against the parsed documentation
def test_env_vars_match_module_utils_convention(self): """Env var names must follow the FLIGHTCTL_ prefix convention used in module_utils/core.py.""" - for option_name, expected_env in self.EXPECTED_ENV_VARS.items(): + for option_name in self.EXPECTED_ENV_VARS: with self.subTest(option=option_name): - self.assertTrue(expected_env.startswith('FLIGHTCTL_'), - f"Env var '{expected_env}' should start with FLIGHTCTL_") + for entry in self.options[option_name].get('env', []): + self.assertTrue( + entry['name'].startswith('FLIGHTCTL_'), + f"Env var '{entry['name']}' should start with FLIGHTCTL_")As per path instructions: "Assertions must be specific — not just 'no exception was raised'".
🤖 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/inventory/test_flightctl.py` around lines 955 - 960, Update test_env_vars_match_module_utils_convention to inspect the plugin’s parsed DOCUMENTATION env definitions rather than iterating over the locally defined EXPECTED_ENV_VARS literals. Extract the declared environment variable names from DOCUMENTATION and assert each actual value starts with FLIGHTCTL_, preserving a specific assertion that fails for missing or incorrectly named declarations.Source: Path instructions
🤖 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 276-286: Update _fetch_auth_config to emit a clear warning before
calling open_url when verify_ssl is false, indicating that TLS certificate
verification is disabled and credentials may be exposed during the OIDC
exchange. Keep the existing validated path and request behavior unchanged.
- Around line 310-320: Replace the hardcoded 10-second timeouts in the OIDC
authentication flow with the resolved request_timeout value. Update the callers
and signatures of _oidc_password_grant and _fetch_auth_config to accept and
propagate that timeout, and use it for every open_url call involved in
authentication while preserving the configured default and existing SSL/CA
handling.
- Around line 623-629: Update _get_data_raw to validate the RESTResponse HTTP
status before processing its body, and handle non-success responses by parsing
structured JSON errors when possible or raising a FlightctlApiException for
non-JSON bodies. Guard json.loads(response.data) against JSONDecodeError, and
ensure error payloads without items are not treated as an empty inventory;
preserve normal pagination for successful responses.
In `@tests/unit/plugins/inventory/test_flightctl.py`:
- Around line 858-888: Move test_oidc_grant_passes_ca_path_to_open_url from
TestSetupConnectionOidcIntegration into TestOidcPasswordGrant, remove its local
_resp helper, and use the existing self._mock_response helper when configuring
mock_open.side_effect. Preserve the current CA-path and certificate-validation
assertions.
- Around line 966-978: In
tests/unit/plugins/inventory/test_flightctl.py:966-978, scope the try/except
ImportError around only the pydantic import, then use
assertRaises(ValidationError) for StrictModel construction and assert
_is_pydantic_validation_error on the captured exception. In
tests/unit/plugins/inventory/test_flightctl.py:1053-1065, apply the same
import-only scoping and return None only when the import fails, so unexpected
successful construction raises normally.
- Around line 1076-1088: Strengthen test_pydantic_error_triggers_fallback by
asserting the exact arguments passed to fallback_func, including label_list,
field_list, limit, headers, and request_timeout. Keep the existing result
assertions, and use the values supplied to _get_data so the test verifies
selector and request-option forwarding through the fallback path.
- Around line 632-663: Add unit tests alongside test_no_oidc_provider_raises for
_oidc_password_grant covering auth configurations whose selected provider spec
omits issuer and clientId. Mock discovery responses with each field absent,
assert ValidationException is raised, and pin the corresponding user-facing
error text for both branches.
---
Outside diff comments:
In `@tests/unit/plugins/inventory/test_flightctl.py`:
- Around line 557-563: Update test_no_basic_auth_ever_produced to use a
configuration containing both a non-null access token and username/password
credentials, then assert that _build_auth_headers returns only the expected
Bearer header and no Basic-Auth credentials. Keep test_no_headers_when_no_token
unchanged so each test covers a distinct behavior.
- Around line 923-953: Remove the eight per-option test methods from the test
class: test_token_env_declaration, test_host_env_declaration,
test_username_env_declaration, test_password_env_declaration,
test_organization_env_declaration, test_config_file_env_declaration,
test_ca_path_env_declaration, and test_verify_ssl_env_declaration. Retain
test_all_connection_options_have_env_declarations as the single table-driven
source of truth for environment declarations.
- Around line 955-960: Update test_env_vars_match_module_utils_convention to
inspect the plugin’s parsed DOCUMENTATION env definitions rather than iterating
over the locally defined EXPECTED_ENV_VARS literals. Extract the declared
environment variable names from DOCUMENTATION and assert each actual value
starts with FLIGHTCTL_, preserving a specific assertion that fails for missing
or incorrectly named declarations.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0aba135e-cf8e-4af7-9889-4c2a5a3fe378
📒 Files selected for processing (2)
plugins/inventory/flightctl.pytests/unit/plugins/inventory/test_flightctl.py
| def _fetch_auth_config(self, host: str, verify_ssl: bool, | ||
| ca_path: str | None = None) -> dict: | ||
| auth_config_url = host.rstrip('/') + "/api/v1/auth/config" | ||
| try: | ||
| resp = open_url(auth_config_url, validate_certs=verify_ssl, | ||
| ca_path=ca_path, timeout=10) | ||
| return json.loads(resp.read()) | ||
| except Exception as exc: | ||
| raise ValidationException( | ||
| f"Failed to fetch auth config from {auth_config_url}: {exc}" | ||
| ) from exc |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Warn the user when TLS verification is disabled during the OIDC exchange.
verify_ssl reaches open_url as validate_certs. In _setup_connection_configuration, verify_ssl falls back to False when the user omits the option (lines 389-394). The plugin now sends the username and password to the auth-config endpoint and the token endpoint. With validate_certs=False, those credentials travel over a channel that is not authenticated, so an active network attacker can present any certificate and capture the password grant. The previous code never sent the password itself from this plugin, so this changed path raises the impact.
Severity: major. The plugin still works, but the security guarantee for credential transport is silently degraded.
The path instructions require a warning when verification is disabled. Emit one before the first request.
🔒️ Proposed warning on the unverified path
def _fetch_auth_config(self, host: str, verify_ssl: bool,
ca_path: str | None = None) -> dict:
auth_config_url = host.rstrip('/') + "/api/v1/auth/config"
+ if not verify_ssl:
+ self._display.warning(
+ "verify_ssl is disabled; the OIDC password grant will send "
+ "credentials without validating the server certificate."
+ )
try:As per path instructions: "Validate TLS certificates by default; warn when verification is disabled".
Also applies to: 355-360
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 276-276: Boolean-typed positional argument in function definition
(FBT001)
[warning] 284-286: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 276 - 286, Update
_fetch_auth_config to emit a clear warning before calling open_url when
verify_ssl is false, indicating that TLS certificate verification is disabled
and credentials may be exposed during the OIDC exchange. Keep the existing
validated path and request behavior unchanged.
Source: Path instructions
| def _oidc_password_grant(self, host: str, username: str, password: str, | ||
| verify_ssl: bool, ca_path: str | None = None) -> str: | ||
| import urllib.error | ||
| import urllib.parse | ||
|
|
||
| auth_config = self._fetch_auth_config(host, verify_ssl, ca_path) | ||
| provider = self._select_oidc_provider(auth_config) | ||
| spec = provider.get("spec", {}) | ||
| issuer = spec.get("issuer") | ||
| client_id = spec.get("clientId") | ||
| scopes = spec.get("scopes") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse the configured request_timeout instead of the hardcoded 10 seconds.
Every open_url call in this flow uses timeout=10. The plugin already exposes a request_timeout option with a default of 120.0, and _setup_connection_configuration reads it. A user who raises request_timeout for a slow IdP still gets a 10-second failure on authentication, and the inventory run aborts before any API call happens. The consequence is an inconsistent and undiscoverable timeout contract across the same plugin run.
Pass the resolved timeout into _oidc_password_grant and _fetch_auth_config.
Also applies to: 331-344
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 311-311: 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/inventory/flightctl.py` around lines 310 - 320, Replace the hardcoded
10-second timeouts in the OIDC authentication flow with the resolved
request_timeout value. Update the callers and signatures of _oidc_password_grant
and _fetch_auth_config to accept and propagate that timeout, and use it for
every open_url call involved in authentication while preserving the configured
default and existing SSL/CA handling.
| except Exception as e: | ||
| raise FlightctlApiException(f"Error retrieving data from Flight Control API: {e}") from e | ||
| data = json.loads(response.data) | ||
| records = data.get('items', []) | ||
| all_records.extend(records) | ||
| metadata = data.get('metadata', {}) | ||
| continue_token = metadata.get('continue', None) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the installed flightctl client REST response type and the without_preload_content contract.
fd -t f 'rest.py' | xargs -r rg -n -C5 'class RESTResponse|def data|self\.status'
fd -t f 'device_api.py|fleet_api.py' | xargs -r rg -n -A20 'def list_devices_without_preload_content|def list_fleets_without_preload_content'Repository: flightctl/flightctl-ansible
Length of output: 165
🌐 Web query:
openapi-generator python without_preload_content return type RESTResponse data status
💡 Result:
In the Python generator for OpenAPI Generator, the without_preload_content method is designed to return a low-level RESTResponse object [1][2]. This approach allows developers to handle the raw response (such as streaming data) without the library automatically loading the entire response body into memory or attempting to deserialize it [3]. Key characteristics of this return type: 1. Return Type: The method returns an instance of RESTResponse [1][2]. In the context of the synchronous generator, this RESTResponse is a wrapper around the underlying HTTP client's response object (typically urllib3.HTTPResponse) [3][4]. For the asynchronous generator, it wraps aiohttp.ClientResponse [5]. 2. Structure: The RESTResponse object provides access to key metadata and data attributes: - status: The HTTP status code of the response [4][5]. - reason: The HTTP reason phrase [4][5]. - headers: A dictionary or dictionary-like object containing the response headers [4][5]. - data: Initially None until the read() method is called [4][5]. - read: A method (or async method in the asyncio generator) that reads and returns the response content [1][4][5]. By using without_preload_content, you bypass the standard deserialization logic, making it the appropriate choice for operations where you need to stream large files or handle raw binary data directly [3][6]. In contrast, standard request methods return deserialized data models or an ApiResponse object that has already consumed the stream [2][7].
Citations:
- 1: [python] fix typing for API responses OpenAPITools/openapi-generator#16802
- 2: https://github.com/OpenAPITools/openapi-generator/blob/8a97ea6d/modules/openapi-generator/src/main/resources/python/api.mustache
- 3: [REQ][python] Allow streaming large response bodies OpenAPITools/openapi-generator#16640
- 4: https://github.com/OpenAPITools/openapi-generator/blob/8a97ea6d/modules/openapi-generator/src/main/resources/python/rest.mustache
- 5: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/python/asyncio/rest.mustache
- 6: [Python] Skip utf8 decoding for specific Content-Types in py3 OpenAPITools/openapi-generator#206
- 7: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/python/api_client.mustache
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant installed package references:\n'
rg -n 'flightctl|flightctl-client|flightctl\.client|from flightctl|import flightctl' pyproject.toml setup.cfg setup.py requirements*.txt poetry.lock uv.lock . 2>/dev/null | sed -n '1,160p' || true
printf '\nInventory file outline:\n'
ast-grep outline plugins/inventory/flightctl.py --view expanded 2>/dev/null | sed -n '1,220p' || true
printf '\nInspect _get_data_raw and callers:\n'
sed -n '560,740p' plugins/inventory/flightctl.py | cat -n | sed 's/^/plugins\/inventory\/flightctl.py:/'
printf '\nSearch for generated flightctl client package metadata:\n'
python3 - <<'PY'
import importlib.util
import json, os, pathlib, sys
mod_names = ['flightctl.client.rest', 'flightctl', 'flightctl.client']
for name in mod_names:
spec = importlib.util.find_spec(name)
print(name, bool(spec), getattr(spec, 'origin', None), getattr(spec, 'submodule_search_locations', None))
PY
PY3=$(command -v python3)
printf '\nPython module files matching flightctl client rest:\n'
$PY3 - <<'PY'
import sys
import os.path as ospath
from pathlib import Path
site=sys.prefix
for root, dirs, files in os.walk(site):
for name in files:
if name == 'rest.py' or name == 'api_client.py':
path=os.path.join(root, name)
text=pathlib.Path(path).read_text(errors='ignore')
if 'RESTResponse' in text or 'without_preload_content' in text or 'preload_content=False' in text:
print(path)
for i, line in enumerate(text.splitlines(), 1):
if 'RESTResponse' in line or 'without_preload_content' in line or 'class RESTResponse' in line or 'def data' in line or '.status' in line:
print(f'{path}:{i}: {line.strip()}')
PYRepository: flightctl/flightctl-ansible
Length of output: 32990
🌐 Web query:
flightctl-client 1.2.1 Python source RESTResponse without_preload_content data status
💡 Result:
In the Python client library for flightctl (such as flightctl-client v1.2.1, which is generated using the OpenAPI Generator [1][2]), the without_preload_content suffix is a standard pattern for API endpoint methods that bypasses automatic deserialization and validation of the response body [3]. When you call an API method with the _without_preload_content suffix, it returns a RESTResponse object rather than a high-level model object [3][4]. Key characteristics of this object include: 1. Data Access: The raw response body is available via the .data attribute, which typically returns a Python byte string [3]. You must decode this manually if you need the content as text or JSON (e.g., json.loads(response.data.decode("utf-8"))) [5]. 2. Status Information: The RESTResponse object provides access to the HTTP status code and response headers, allowing you to inspect the raw server response [3][4]. 3. No Validation: Unlike standard API calls that use Pydantic or other schema validation tools to map JSON to objects, the without_preload_content variant returns the response stream directly [3][6]. This is useful for handling large payloads [6], streaming data [7], or bypassing strict client-side validation when working with legacy or unexpected API responses [5]. If you require both the parsed model and the raw HTTP information (status, headers), you should instead use the _with_http_info variant of the method, which provides an ApiResponse container [3][4]. Note that without_preload_content methods are specifically designed for scenarios where you wish to manage the raw response handling yourself [3][5].
Citations:
- 1: https://pypi.org/project/flightctl-client/1.2.1/
- 2: https://pypi.org/project/flightctl-client/
- 3: https://stackoverflow.com/questions/75324883/how-can-the-client-side-validation-be-disabled-in-the-python-code-generated-from
- 4: [python] fix typing for API responses OpenAPITools/openapi-generator#16802
- 5: https://github.com/coval-ai/coval-examples/blob/main/python-sdk/examples/list_agents.py
- 6: [REQ][python] Allow streaming large response bodies OpenAPITools/openapi-generator#16640
- 7: https://github.com/KeiichiHirobe/package-test
🌐 Web query:
site:github.com/flightctl-client Python RESTResponse "self.status" "self.data" "without_preload_content"
💡 Result:
Found it in flightctl/flightctl-python-client:
- File:
flightctl/rest.py RESTResponseclass: lines 755–784self.statusandself.dataare initialized at lines 761 and 765.- The HTTP request uses
preload_content=FalsethroughoutRESTClientObject.request()and returnsRESTResponse(r)at line 1131. (github.com)
The repository is flightctl/flightctl-python-client. (github.com)
Citations:
- 1: https://github.com/flightctl/flightctl-python-client/blob/main/flightctl/rest.py
- 2: https://github.com/flightctl/python-client
Validate the HTTP status and guard the JSON decode in _get_data_raw.
without_preload_content returns the raw FlightControl client RESTResponse, so handle status and body content as the caller. If the API returns 401/500, parse the body as JSON after checking the status, otherwise raise or handle the non-JSON response to avoid unhandled json.JSONDecodeError. If an error body lacks items, return or raise instead of treating it as an empty inventory.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 624-624: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 623 - 629, Update _get_data_raw
to validate the RESTResponse HTTP status before processing its body, and handle
non-success responses by parsing structured JSON errors when possible or raising
a FlightctlApiException for non-JSON bodies. Guard json.loads(response.data)
against JSONDecodeError, and ensure error payloads without items are not treated
as an empty inventory; preserve normal pagination for successful responses.
| @patch(OPEN_URL) | ||
| def test_no_oidc_provider_raises(self, mock_open): | ||
| mock_open.return_value = self._mock_response({"providers": []}) | ||
| with self.assertRaises(ValidationException) as ctx: | ||
| self.module._oidc_password_grant( | ||
| "https://host", "admin", "pass", False | ||
| ) | ||
| self.assertIn("No OIDC provider found", str(ctx.exception)) | ||
|
|
||
| @patch(OPEN_URL) | ||
| def test_oidc_discovery_failure_raises(self, mock_open): | ||
| mock_open.side_effect = [ | ||
| self._mock_response(self.SAMPLE_AUTH_CONFIG), | ||
| Exception("Connection refused"), | ||
| ] | ||
| with self.assertRaises(ValidationException) as ctx: | ||
| self.module._oidc_password_grant( | ||
| "https://host", "admin", "pass", False | ||
| ) | ||
| self.assertIn("OIDC discovery failed", str(ctx.exception)) | ||
|
|
||
| @patch(OPEN_URL) | ||
| def test_missing_token_endpoint_raises(self, mock_open): | ||
| mock_open.side_effect = [ | ||
| self._mock_response(self.SAMPLE_AUTH_CONFIG), | ||
| self._mock_response({}), | ||
| ] | ||
| with self.assertRaises(ValidationException) as ctx: | ||
| self.module._oidc_password_grant( | ||
| "https://host", "admin", "pass", False | ||
| ) | ||
| self.assertIn("token_endpoint missing", str(ctx.exception)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the missing issuer and missing clientId branches.
_oidc_password_grant raises ValidationException when the selected provider spec has no issuer (flightctl.py lines 322-325) or no clientId (lines 326-329). No test exercises either branch. Both are reachable with a real auth config from a partially configured Flight Control server, and both produce user-facing error text that nothing currently pins.
The consequence is that a later refactor could drop either guard, or change the message a user relies on, and the suite stays green.
💚 Proposed additional tests
+ `@patch`(OPEN_URL)
+ def test_missing_issuer_raises(self, mock_open):
+ config = {"providers": [{"metadata": {"name": "p"},
+ "spec": {"providerType": "oidc", "clientId": "c"}}]}
+ mock_open.return_value = self._mock_response(config)
+ with self.assertRaises(ValidationException) as ctx:
+ self.module._oidc_password_grant("https://host", "admin", "pass", False)
+ self.assertIn("no issuer URL", str(ctx.exception))
+
+ `@patch`(OPEN_URL)
+ def test_missing_client_id_raises(self, mock_open):
+ config = {"providers": [{"metadata": {"name": "p"},
+ "spec": {"providerType": "oidc",
+ "issuer": "https://idp.example.com"}}]}
+ mock_open.return_value = self._mock_response(config)
+ with self.assertRaises(ValidationException) as ctx:
+ self.module._oidc_password_grant("https://host", "admin", "pass", False)
+ self.assertIn("no clientId", str(ctx.exception))As per path instructions: "Test both success and error/exception paths".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @patch(OPEN_URL) | |
| def test_no_oidc_provider_raises(self, mock_open): | |
| mock_open.return_value = self._mock_response({"providers": []}) | |
| with self.assertRaises(ValidationException) as ctx: | |
| self.module._oidc_password_grant( | |
| "https://host", "admin", "pass", False | |
| ) | |
| self.assertIn("No OIDC provider found", str(ctx.exception)) | |
| @patch(OPEN_URL) | |
| def test_oidc_discovery_failure_raises(self, mock_open): | |
| mock_open.side_effect = [ | |
| self._mock_response(self.SAMPLE_AUTH_CONFIG), | |
| Exception("Connection refused"), | |
| ] | |
| with self.assertRaises(ValidationException) as ctx: | |
| self.module._oidc_password_grant( | |
| "https://host", "admin", "pass", False | |
| ) | |
| self.assertIn("OIDC discovery failed", str(ctx.exception)) | |
| @patch(OPEN_URL) | |
| def test_missing_token_endpoint_raises(self, mock_open): | |
| mock_open.side_effect = [ | |
| self._mock_response(self.SAMPLE_AUTH_CONFIG), | |
| self._mock_response({}), | |
| ] | |
| with self.assertRaises(ValidationException) as ctx: | |
| self.module._oidc_password_grant( | |
| "https://host", "admin", "pass", False | |
| ) | |
| self.assertIn("token_endpoint missing", str(ctx.exception)) | |
| `@patch`(OPEN_URL) | |
| def test_no_oidc_provider_raises(self, mock_open): | |
| mock_open.return_value = self._mock_response({"providers": []}) | |
| with self.assertRaises(ValidationException) as ctx: | |
| self.module._oidc_password_grant( | |
| "https://host", "admin", "pass", False | |
| ) | |
| self.assertIn("No OIDC provider found", str(ctx.exception)) | |
| `@patch`(OPEN_URL) | |
| def test_missing_issuer_raises(self, mock_open): | |
| config = {"providers": [{"metadata": {"name": "p"}, | |
| "spec": {"providerType": "oidc", "clientId": "c"}}]} | |
| mock_open.return_value = self._mock_response(config) | |
| with self.assertRaises(ValidationException) as ctx: | |
| self.module._oidc_password_grant("https://host", "admin", "pass", False) | |
| self.assertIn("no issuer URL", str(ctx.exception)) | |
| `@patch`(OPEN_URL) | |
| def test_missing_client_id_raises(self, mock_open): | |
| config = {"providers": [{"metadata": {"name": "p"}, | |
| "spec": {"providerType": "oidc", | |
| "issuer": "https://idp.example.com"}}]} | |
| mock_open.return_value = self._mock_response(config) | |
| with self.assertRaises(ValidationException) as ctx: | |
| self.module._oidc_password_grant("https://host", "admin", "pass", False) | |
| self.assertIn("no clientId", str(ctx.exception)) | |
| `@patch`(OPEN_URL) | |
| def test_oidc_discovery_failure_raises(self, mock_open): | |
| mock_open.side_effect = [ | |
| self._mock_response(self.SAMPLE_AUTH_CONFIG), | |
| Exception("Connection refused"), | |
| ] | |
| with self.assertRaises(ValidationException) as ctx: | |
| self.module._oidc_password_grant( | |
| "https://host", "admin", "pass", False | |
| ) | |
| self.assertIn("OIDC discovery failed", str(ctx.exception)) | |
| `@patch`(OPEN_URL) | |
| def test_missing_token_endpoint_raises(self, mock_open): | |
| mock_open.side_effect = [ | |
| self._mock_response(self.SAMPLE_AUTH_CONFIG), | |
| self._mock_response({}), | |
| ] | |
| with self.assertRaises(ValidationException) as ctx: | |
| self.module._oidc_password_grant( | |
| "https://host", "admin", "pass", False | |
| ) | |
| self.assertIn("token_endpoint missing", str(ctx.exception)) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 635-635: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 637-637: Boolean positional value in function call
(FBT003)
[warning] 647-647: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 649-649: Boolean positional value in function call
(FBT003)
[warning] 659-659: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 661-661: Boolean positional value in function call
(FBT003)
🤖 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/inventory/test_flightctl.py` around lines 632 - 663, Add
unit tests alongside test_no_oidc_provider_raises for _oidc_password_grant
covering auth configurations whose selected provider spec omits issuer and
clientId. Mock discovery responses with each field absent, assert
ValidationException is raised, and pin the corresponding user-facing error text
for both branches.
Source: Path instructions
| @patch("plugins.inventory.flightctl.open_url") | ||
| def test_oidc_grant_passes_ca_path_to_open_url(self, mock_open): | ||
| def _resp(body): | ||
| r = MagicMock() | ||
| r.read.return_value = json.dumps(body).encode() | ||
| return r | ||
|
|
||
| auth_config = { | ||
| "providers": [{ | ||
| "metadata": {"name": "p"}, | ||
| "spec": { | ||
| "providerType": "oidc", | ||
| "issuer": "https://idp.example.com", | ||
| "clientId": "c", | ||
| }, | ||
| }], | ||
| } | ||
| mock_open.side_effect = [ | ||
| _resp(auth_config), | ||
| _resp({"token_endpoint": "https://idp.example.com/token"}), | ||
| _resp({"id_token": "tok"}), | ||
| ] | ||
|
|
||
| module = InventoryModule() | ||
| module._oidc_password_grant( | ||
| "https://host", "admin", "pass", True, "/path/to/ca.crt" | ||
| ) | ||
|
|
||
| for call in mock_open.call_args_list: | ||
| self.assertTrue(call[1].get("validate_certs")) | ||
| self.assertEqual(call[1].get("ca_path"), "/path/to/ca.crt") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Move this test into TestOidcPasswordGrant and drop the duplicated response helper.
The class docstring at line 767 states that TestSetupConnectionOidcIntegration verifies _setup_connection_configuration. This test calls _oidc_password_grant directly and never touches _setup_connection_configuration, so it sits in the wrong class. It also redefines _resp, which is a byte-for-byte copy of TestOidcPasswordGrant._mock_response.
The consequence is a DRY violation plus a misleading test taxonomy: someone auditing OIDC transport coverage reads TestOidcPasswordGrant, sees no TLS assertions, and concludes the CA-path behavior is untested.
Move the test to TestOidcPasswordGrant and call self._mock_response.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 861-861: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.16.1)
[warning] 860-860: Missing return type annotation for private function _resp
(ANN202)
[warning] 883-883: Boolean positional value in function call
(FBT003)
🤖 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/inventory/test_flightctl.py` around lines 858 - 888, Move
test_oidc_grant_passes_ca_path_to_open_url from
TestSetupConnectionOidcIntegration into TestOidcPasswordGrant, remove its local
_resp helper, and use the existing self._mock_response helper when configuring
mock_open.side_effect. Preserve the current CA-path and certificate-validation
assertions.
| def test_pydantic_error_triggers_fallback(self): | ||
| pydantic_exc = self._make_pydantic_error() | ||
| if pydantic_exc is None: | ||
| self.skipTest("pydantic not installed") | ||
|
|
||
| list_func = MagicMock(side_effect=pydantic_exc) | ||
| raw_items = [{'metadata': {'name': 'dev-1'}}] | ||
| fallback_func = MagicMock(return_value=self._make_raw_response(raw_items)) | ||
|
|
||
| result = _get_data(list_func, fallback_list_func=fallback_func) | ||
| self.assertEqual(len(result), 1) | ||
| self.assertEqual(result[0]['metadata']['name'], 'dev-1') | ||
| fallback_func.assert_called_once() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the arguments forwarded to the fallback, not just that it was called.
fallback_func.assert_called_once() passes regardless of what _get_data forwards. _get_data passes label_list, field_list, limit, headers, and request_timeout into _get_data_raw (flightctl.py lines 667-674), and _get_devices_by_labels_and_fields depends on those selectors reaching the raw endpoint.
If the forwarding of field_list broke, the fallback would fetch every device instead of the filtered set. _fetch_fleet_devices would then place unrelated devices into a fleet group, and the inventory would be silently wrong rather than failing. This test would still pass.
Pin the selectors in the assertion.
💚 Proposed argument assertion
- result = _get_data(list_func, fallback_list_func=fallback_func)
+ result = _get_data(
+ list_func,
+ field_list="metadata.owner = Fleet/f1",
+ limit=50,
+ fallback_list_func=fallback_func,
+ )
self.assertEqual(len(result), 1)
self.assertEqual(result[0]['metadata']['name'], 'dev-1')
- fallback_func.assert_called_once()
+ fallback_func.assert_called_once()
+ kwargs = fallback_func.call_args[1]
+ self.assertEqual(kwargs['field_selector'], "metadata.owner = Fleet/f1")
+ self.assertEqual(kwargs['limit'], 50)As per path instructions: "Assertions must be specific — not just 'no exception was raised'".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_pydantic_error_triggers_fallback(self): | |
| pydantic_exc = self._make_pydantic_error() | |
| if pydantic_exc is None: | |
| self.skipTest("pydantic not installed") | |
| list_func = MagicMock(side_effect=pydantic_exc) | |
| raw_items = [{'metadata': {'name': 'dev-1'}}] | |
| fallback_func = MagicMock(return_value=self._make_raw_response(raw_items)) | |
| result = _get_data(list_func, fallback_list_func=fallback_func) | |
| self.assertEqual(len(result), 1) | |
| self.assertEqual(result[0]['metadata']['name'], 'dev-1') | |
| fallback_func.assert_called_once() | |
| def test_pydantic_error_triggers_fallback(self): | |
| pydantic_exc = self._make_pydantic_error() | |
| if pydantic_exc is None: | |
| self.skipTest("pydantic not installed") | |
| list_func = MagicMock(side_effect=pydantic_exc) | |
| raw_items = [{'metadata': {'name': 'dev-1'}}] | |
| fallback_func = MagicMock(return_value=self._make_raw_response(raw_items)) | |
| result = _get_data( | |
| list_func, | |
| field_list="metadata.owner = Fleet/f1", | |
| limit=50, | |
| fallback_list_func=fallback_func, | |
| ) | |
| self.assertEqual(len(result), 1) | |
| self.assertEqual(result[0]['metadata']['name'], 'dev-1') | |
| fallback_func.assert_called_once() | |
| kwargs = fallback_func.call_args[1] | |
| self.assertEqual(kwargs['field_selector'], "metadata.owner = Fleet/f1") | |
| self.assertEqual(kwargs['limit'], 50) |
🤖 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/inventory/test_flightctl.py` around lines 1076 - 1088,
Strengthen test_pydantic_error_triggers_fallback by asserting the exact
arguments passed to fallback_func, including label_list, field_list, limit,
headers, and request_timeout. Keep the existing result assertions, and use the
values supplied to _get_data so the test verifies selector and request-option
forwarding through the fallback path.
Source: Path instructions
These changes are now in a dedicated PR #63 on branch bugfix/EDM-4980-pydantic-fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/inventory/flightctl.py (1)
372-372: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrefer
access_tokenfor the Bearer credential.When the OIDC token endpoint returns both fields,
id_token | access_tokenstores the ID token asConfiguration.access_token, and API calls use that token in the Bearer header. If the ID token is not valid as an API access token, authentication can fail. Selectaccess_tokenfirst and keepid_tokenonly as a fallback if Flight Control explicitly supports it.Proposed fix
- token = data.get("id_token") or data.get("access_token") + token = data.get("access_token") or data.get("id_token")Add a unit test that returns both fields and asserts
Configuration.access_tokenreceives theaccess_token.🤖 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` at line 372, Update the token selection in the OIDC response handling to prefer data.get("access_token") and use data.get("id_token") only as the fallback, so Configuration.access_token receives the API credential when both exist. Add a unit test covering a response containing both fields and assert that Configuration.access_token stores the access_token value.
🤖 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`:
- Line 450: Update the fleet conversion loop in _populate_inventory_fleets to
accept both fleet model objects and raw dictionaries, avoiding to_dict() when
the item is already a dictionary while preserving model serialization. Add a
test covering a raw fleet dictionary passed to _populate_inventory_fleets.
---
Outside diff comments:
In `@plugins/inventory/flightctl.py`:
- Line 372: Update the token selection in the OIDC response handling to prefer
data.get("access_token") and use data.get("id_token") only as the fallback, so
Configuration.access_token receives the API credential when both exist. Add a
unit test covering a response containing both fields and assert that
Configuration.access_token stores the access_token value.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cf4692fc-f327-40d8-992f-10ac76f40f1f
📒 Files selected for processing (2)
plugins/inventory/flightctl.pytests/unit/plugins/inventory/test_flightctl.py
| @@ -334,6 +448,7 @@ def _populate_inventory_fleets(self, fleets: List[Any], config) -> None: | |||
| return | |||
|
|
|||
| for fleet in [fleet.to_dict() for fleet in fleets]: | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve support for raw fleet dictionaries.
Line 450 calls to_dict() for every fleet. A raw dictionary raises AttributeError and aborts inventory generation. This breaks the stated model-or-dictionary fleet contract.
Proposed fix
- for fleet in [fleet.to_dict() for fleet in fleets]:
+ for raw_fleet in fleets:
+ fleet = raw_fleet.to_dict() if hasattr(raw_fleet, "to_dict") else raw_fleet
fleet = _convert_enums_to_strings(fleet)Add a test that passes a raw fleet dictionary to _populate_inventory_fleets.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for fleet in [fleet.to_dict() for fleet in fleets]: | |
| for raw_fleet in fleets: | |
| fleet = raw_fleet.to_dict() if hasattr(raw_fleet, "to_dict") else raw_fleet | |
| fleet = _convert_enums_to_strings(fleet) |
🤖 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` at line 450, Update the fleet conversion loop
in _populate_inventory_fleets to accept both fleet model objects and raw
dictionaries, avoiding to_dict() when the item is already a dictionary while
preserving model serialization. Add a test covering a raw fleet dictionary
passed to _populate_inventory_fleets.
|
One thing worth flagging: this same issue likely affects the other modules too. plugins/module_utils/api_module.py (set_auth()) and plugins/module_utils/imagebuilder_module.py (_set_auth_headers()) both still send HTTP Basic Auth for username/password — so flightctl_resource, flightctl_resource_info, flightctl_certificate_management, flightctl_enrollment_config_info, and the image builder modules have the same issue. Might be worth extracting the OIDC discovery/password-grant logic from _oidc_password_grant() into a shared helper (e.g. plugins/module_utils/oidc_auth.py) so it can be reused by api_module.py and imagebuilder_module.py as well |
2 participants
Summary
_oidc_password_grant()method that discovers the token endpoint via OIDC.well-known/openid-configurationand exchanges credentials for a JWT Bearer token, matching theflightctlCLI behavior_build_auth_headers()and updated DOCUMENTATION descriptions forusername/passwordoptionsTest plan
🤖 Generated with Claude Code
Affected areas
plugins/inventory/flightctl.py.well-known/openid-configuration.access_tokenresponse field as a Bearer token.ca_pathto HTTPS requests.usernameandpassworddocumentation.tests/unit/plugins/inventory/test_flightctl.pyAPI and compatibility
Unaffected areas
No changes are reported for modules, shared utilities, connection plugins, integration tests, demo files, CI configuration, collection metadata, or changelogs.
Validation
Manual testing with a RHEM instance remains pending.