Skip to content

Add unit tests for osism/api.py helpers and Pydantic models#2463

Open
berendt wants to merge 2 commits into
mainfrom
unit-tests-api-helpers
Open

Add unit tests for osism/api.py helpers and Pydantic models#2463
berendt wants to merge 2 commits into
mainfrom
unit-tests-api-helpers

Conversation

@berendt

@berendt berendt commented Jul 13, 2026

Copy link
Copy Markdown
Member

Adds tests/unit/test_api_helpers.py covering the module-level helpers and Pydantic models of the OSISM API server (osism/api.py), as specified in #2359:

  • _mask_inventory_secrets() — secret-key matching (password/secret substrings, ironic_osism_ prefix, using the real _is_secret_key), Ansible Vault values (including leading whitespace, and the marker mid-string staying unchanged), nested dict recursion, secret keys with dict values masked as a whole, pass-through of non-dict/non-string values and non-string keys, non-traversal of lists, empty dict, input immutability.
  • find_device_by_identifier() — lookup precedence namecf_inventory_hostnameserial with call-order assertions, early None return without a NetBox connection, first-match semantics for multiple results. utils.nb is injected via patch.dict("osism.utils.__dict__", ...) to avoid triggering the lazy module __getattr__, matching the existing pattern in tests/unit/commands/.
  • process_netbox_webhook()reconciler.run.delay() dispatched exactly once for managed servers; no dispatch for switches, interfaces (device fetched via nb.dcim.devices.get(id=...), asserted), device_type fallback to "node", unknown URLs (early return without NetBox access), and unmanaged devices; KeyError propagation for payloads without url/name.
  • LogConfig — defaults, model_dump() structure (formatter, StreamHandler on ext://sys.stderr, all five loggers), dictConfig compatibility.
  • Pydantic modelsNotificationBaremetal and WebhookNetboxData validation and coercion (UUID, datetime, per-field required checks), BaremetalNode optional fields and default_factory independence, all-optional models, and parametrized required-field enforcement plus model_dump() round trips for the response models, including dict-to-BaremetalNodeNetboxInfo coercion and Any-typed value fields.

Endpoint behavior (HTTP status codes, masking applied in responses) is out of scope here and belongs to the companion endpoint issue (Tier 7, split 2/2).

Closes #2359

🤖 Generated with Claude Code

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@berendt berendt force-pushed the unit-tests-api-helpers branch from bdcace2 to 03dbbaa Compare July 13, 2026 20:35
@berendt berendt requested a review from ideaship July 15, 2026 08:08
@berendt berendt moved this from New to Ready for review in Human Board Jul 15, 2026
@github-project-automation github-project-automation Bot moved this from Ready for review to In review in Human Board Jul 15, 2026
@ideaship ideaship dismissed a stale review July 15, 2026 12:22

Dismissing — this was meant to be a pending/draft review, not a submitted one. Reposting as a draft.

Comment thread tests/unit/test_api_helpers.py Outdated
assert api._mask_inventory_secrets({"key": value}) == {"key": value}


def test_mask_inventory_secrets_does_not_traverse_lists():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test asserts, as expected behavior, that a secret nested inside a list is returned unmasked:

data = {"hosts": [{"db_password": "hunter2"}]}
assert api._mask_inventory_secrets(data) == {"hosts": [{"db_password": "hunter2"}]}

That codifies a production leak. _mask_inventory_secrets (osism/api.py:35-51) recurses into dict but has no list branch, and it masks the ansible-inventory --host output returned by the API at api.py:833, 903, and 1104. Ansible hostvars routinely contain lists of dicts (e.g. users: [{name: admin, password: ...}]), so any secret nested in a list is returned verbatim through /hostvars and the search endpoints — the masking layer that exists to prevent exactly this is silently bypassed.

The sibling masker _mask_secrets_inplace (osism/tasks/conductor/utils.py:314-320) already has an isinstance(obj, list) branch, so the missing list handling here is an oversight, not an intentional design choice.

Fix _mask_inventory_secrets to traverse lists (mirroring _mask_secrets_inplace), and flip this assertion to expect {"hosts": [{"db_password": "***"}]}. Keep the production fix in its own commit, separate from the test changes. As written, the test would actively resist that fix.

berendt added 2 commits July 16, 2026 14:04
_mask_inventory_secrets() recursed into dicts but not into lists, so
secrets nested inside lists (for example users: [{password: ...}] in
Ansible hostvars) were returned unmasked by the /hostvars and search
endpoints. Traverse lists as well and mask secret keys and Ansible
Vault encrypted values at any nesting depth, mirroring
_mask_secrets_inplace() in osism/tasks/conductor/utils.py.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Christian Berendt <berendt@osism.tech>
Cover the module-level helper functions and request/response models of
the OSISM API server in a new tests/unit/test_api_helpers.py:

- _mask_inventory_secrets(): secret-key matching (password/secret
  substrings, ironic_osism_ prefix), Ansible Vault values including
  leading whitespace, recursion into nested dicts and lists (including
  Vault values inside lists), pass-through of non-dict/non-string
  values, non-string keys, and input immutability
- find_device_by_identifier(): lookup precedence name ->
  cf_inventory_hostname -> serial, early return without a NetBox
  connection, first-match semantics
- process_netbox_webhook(): reconciler.run.delay() dispatch for managed
  servers, log-only branches for switches and interfaces, device_type
  fallback to "node", unknown URL early return, unmanaged devices,
  KeyError propagation for incomplete payloads
- LogConfig: defaults, model_dump() structure, dictConfig compatibility
- Pydantic models: validation, coercion (UUID, datetime, nested
  models), defaults and default_factory independence, required-field
  enforcement, model_dump() round trips

Endpoint behavior (HTTP status codes, masking in responses) is covered
separately with the FastAPI TestClient.

Closes #2359

Assisted-by: Claude:claude-fable-5
Signed-off-by: Christian Berendt <berendt@osism.tech>
@berendt berendt force-pushed the unit-tests-api-helpers branch from 03dbbaa to e6a91ed Compare July 16, 2026 12:04
@berendt berendt requested a review from ideaship July 16, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

Unit tests for osism/api.py — helper functions, Pydantic models, secret masking

3 participants