Skip to content

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

Merged
ideaship merged 2 commits into
mainfrom
unit-tests-api-helpers
Jul 17, 2026
Merged

Add unit tests for osism/api.py helpers and Pydantic models#2463
ideaship merged 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
@ideaship
ideaship merged commit 0670b9e into main Jul 17, 2026
3 checks passed
@ideaship
ideaship deleted the unit-tests-api-helpers branch July 17, 2026 09:31
@github-project-automation github-project-automation Bot moved this from In review to Done in Human Board Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

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

3 participants