test: add MaaS API key tests#1244
Merged
sheltoncyril merged 6 commits intoopendatahub-io:mainfrom Mar 18, 2026
Merged
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/verified', '/cherry-pick', '/hold', '/lgtm', '/build-push-pr-image'} |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a MaaS API key CRUD test suite, three pytest fixtures for key lifecycle, and three HTTP helper functions (get/list/revoke) using Bearer auth, URL encoding, and strict JSON parsing; tests cover create (show-once), list (filters/sort/pagination), and revoke flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Actionable Issues
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py (1)
27-27: Security coverage gap: CRUD tests run only with admin actor.Current parametrization skips free/premium actor paths available in
tests/model_serving/maas_billing/conftest.py:459-502, so authorization regressions can pass undetected.As per coding guidelines, "**: REVIEW PRIORITIES: 1. Security vulnerabilities ... 2. Architectural issues and anti-patterns".
Also applies to: 67-67, 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` at line 27, The tests only parametrize ocp_token_for_actor as admin, leaving out free/premium actor types and missing authorization paths; update each pytest.mark.parametrize("ocp_token_for_actor", [{"type": "admin"}], indirect=True) occurrence in tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py (and the other two occurrences noted) to include the free and premium actor variants (e.g. pass a list of dicts with {"type":"admin"}, {"type":"free"}, {"type":"premium"}) so the fixture defined in tests/model_serving/maas_billing/conftest.py will exercise admin/free/premium authorization paths across the CRUD tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py`:
- Line 53: The test currently logs secret API key material via LOGGER.info using
the key variable and key[:15]; remove any secret exposure by changing the log to
omit the key and only record non-sensitive identifiers (e.g., use
LOGGER.info(f"[create] Created key id={body['id']}") or log a masked form
produced by a helper like mask_key(key) that does not include raw key bytes),
update references to LOGGER, body['id'], and key accordingly, and ensure no
other tests print key or key[:...] anywhere.
- Around line 28-131: Tests create API keys with create_api_key (in
test_create_api_key and test_list_api_keys) but never remove them; wrap resource
creation in try/finally or use a fixture to always call the cleanup helper
(e.g., delete_api_key) with the returned id(s) and the same
request_session_http/base_url/ocp_user_token so keys are deactivated/removed
after assertions; ensure the delete_api_key call checks the response status
(200/204) and tolerate missing keys on cleanup, and apply the same pattern for
both test_create_api_key (delete the single created id) and test_list_api_keys
(delete key1_id and key2_id) to prevent test-state leakage.
In `@tests/model_serving/maas_billing/maas_subscription/utils.py`:
- Around line 232-233: The key_id is interpolated directly into the path in
get_api_key() and revoke_api_key(), allowing path-control characters to
manipulate endpoints; URL-encode or percent-encode key_id (e.g., via
urllib.parse.quote with safe='' or equivalent) before constructing the url
variable so characters like '/' and '?' are escaped, then use the encoded value
when calling request_session_http.get/post to prevent resource path traversal.
---
Nitpick comments:
In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py`:
- Line 27: The tests only parametrize ocp_token_for_actor as admin, leaving out
free/premium actor types and missing authorization paths; update each
pytest.mark.parametrize("ocp_token_for_actor", [{"type": "admin"}],
indirect=True) occurrence in
tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py (and the
other two occurrences noted) to include the free and premium actor variants
(e.g. pass a list of dicts with {"type":"admin"}, {"type":"free"},
{"type":"premium"}) so the fixture defined in
tests/model_serving/maas_billing/conftest.py will exercise admin/free/premium
authorization paths across the CRUD tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 738df610-02bc-4161-ae81-0242f65786be
📒 Files selected for processing (2)
tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.pytests/model_serving/maas_billing/maas_subscription/utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py (1)
38-65:⚠️ Potential issue | 🟠 MajorGuarantee API key cleanup in
finallyto prevent cross-test leakage.At Line 38-65 and Line 79-131, created keys are left active. This leaks persistent state and can cause quota-driven flakes across runs.
Remediation diff
def test_create_api_key( @@ - _, body = create_api_key( + _, body = create_api_key( base_url=base_url, ocp_user_token=ocp_token_for_actor, request_session_http=request_session_http, api_key_name=key_name, ) + key_id = body["id"] + try: + assert "id" in body, f"Expected 'id' in create response, got: {body}" + assert "key" in body, f"Expected 'key' in create response, got: {body}" + assert "name" in body, f"Expected 'name' in create response, got: {body}" + + key = body["key"] + assert key.startswith("sk-oai-"), f"Expected 'sk-oai-' prefix, got: {key[:20]}" + assert len(key) > len("sk-oai-"), "Key body after prefix must not be empty" + + get_resp, get_body = get_api_key( + request_session_http=request_session_http, + base_url=base_url, + key_id=body["id"], + ocp_user_token=ocp_token_for_actor, + ) + assert get_resp.status_code == 200, ( + f"Expected 200 on GET /v1/api-keys/{body['id']}, got {get_resp.status_code}: {get_resp.text[:200]}" + ) + assert "key" not in get_body, "Plaintext key must not be returned by GET after creation (show-once pattern)" + finally: + revoke_api_key( + request_session_http=request_session_http, + base_url=base_url, + key_id=key_id, + ocp_user_token=ocp_token_for_actor, + ) @@ - _, key1_body = create_api_key( + _, key1_body = create_api_key( base_url=base_url, ocp_user_token=ocp_token_for_actor, request_session_http=request_session_http, api_key_name=key1_name, ) @@ key1_id = key1_body["id"] key2_id = key2_body["id"] - - list_resp, list_body = list_api_keys( + try: + list_resp, list_body = list_api_keys( + request_session_http=request_session_http, + base_url=base_url, + ocp_user_token=ocp_token_for_actor, + filters={"status": ["active"]}, + sort={"by": "created_at", "order": "desc"}, + pagination={"limit": 50, "offset": 0}, + ) + # existing assertions... + finally: + for key_id in (key1_id, key2_id): + revoke_api_key( + request_session_http=request_session_http, + base_url=base_url, + key_id=key_id, + ocp_user_token=ocp_token_for_actor, + ) - request_session_http=request_session_http, - base_url=base_url, - ocp_user_token=ocp_token_for_actor, - filters={"status": ["active"]}, - sort={"by": "created_at", "order": "desc"}, - pagination={"limit": 50, "offset": 0}, - )As per coding guidelines, "**: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".
Also applies to: 79-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` around lines 38 - 65, The test creates an API key with create_api_key and never removes it, causing state leakage; wrap the create/get assertions in a try/finally and in the finally call the existing deletion/revoke helper (e.g., delete_api_key or an equivalent API call using body["id"]) to ensure the key is removed even if assertions fail, handling and logging any deletion errors (and checking response codes) so cleanup won't silently fail; update both the block around create_api_key/get_api_key and the similar block at the later section (79-131) to use the same finally cleanup pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py`:
- Around line 45-47: The test currently interpolates the full response body and
API key into assertion messages (variable body and body["key"]), which may leak
secrets on failure; update the assertions in the test (the checks referencing
body, "id", "key", and "name") to use generic, non-secret messages or masked
info only (e.g., assert "id" in body, assert "key" in body, assert "name" in
body) and replace any f-strings that include body or body["key"] with fixed
strings like "create response missing 'id'/'key'/'name'" or a masked summary (no
actual key material), ensuring no secrets from body are interpolated into
assertion messages or test logs.
- Around line 94-112: The test is flaky because it asserts list membership
immediately after creating keys; modify the block that calls list_api_keys so it
polls/retries until both key1_id and key2_id appear (or a timeout is reached)
instead of asserting immediately. Use list_api_keys as the poll function, check
items = list_body.get("items") or list_body.get("data") or [], and loop with a
short sleep/backoff and a max wait (e.g., retries or timeout) before failing; if
both key1_id and key2_id are present break and assert success, otherwise raise
the existing assertion after timeout.
In `@tests/model_serving/maas_billing/maas_subscription/utils.py`:
- Around line 222-232: Validate the key_id before building the URL in
get_api_key: ensure key_id is not empty or only whitespace and if invalid raise
a clear exception (e.g., ValueError) or return an error response immediately
instead of constructing f"{base_url}/v1/api-keys/{quote(key_id, safe='')}", to
avoid hitting the collection endpoint; apply the same validation to any other
helper that constructs /v1/api-keys/{id} (the similar block noted around lines
285-295) so all calls reject empty/whitespace IDs early with a consistent error.
- Line 238: The log lines write raw key_id which allows log-forging via control
characters; update the LOGGER.info calls that reference key_id (e.g., the
get_api_key logging statement and the similar one at the other occurrence) to
log a sanitized/escaped version instead of the raw value—produce an
escaped_key_id by encoding control characters (for example using
json.dumps(key_id) or replacing/remapping \n, \r, \t and other non-printable
chars to visible escape sequences) and use escaped_key_id in the log message so
no untrusted control characters can inject forged log lines.
---
Duplicate comments:
In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py`:
- Around line 38-65: The test creates an API key with create_api_key and never
removes it, causing state leakage; wrap the create/get assertions in a
try/finally and in the finally call the existing deletion/revoke helper (e.g.,
delete_api_key or an equivalent API call using body["id"]) to ensure the key is
removed even if assertions fail, handling and logging any deletion errors (and
checking response codes) so cleanup won't silently fail; update both the block
around create_api_key/get_api_key and the similar block at the later section
(79-131) to use the same finally cleanup pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: fc487078-7706-4e18-a100-35139a968860
📒 Files selected for processing (2)
tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.pytests/model_serving/maas_billing/maas_subscription/utils.py
dbasunag
requested changes
Mar 17, 2026
rhods-ci-bot
added
changes-requested-by-dbasunag
and removed
changes-requested-by-dbasunag
commented-by-SB159
labels
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
for more information, see https://pre-commit.ci
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py (4)
27-62:⚠️ Potential issue | 🟠 MajorCreated API key is not cleaned up (CWE-459: Incomplete Cleanup).
test_create_api_keycreates a key but never revokes it. Unliketest_list_api_keysandtest_revoke_api_keywhich now use fixtures, this test still has direct creation without teardown.Proposed fix using try/finally
+from tests.model_serving.maas_billing.maas_subscription.utils import revoke_api_key @@ def test_create_api_key( self, request_session_http: requests.Session, base_url: str, ocp_token_for_actor: str, ) -> None: """Verify API key creation and show-once behavior.""" key_name = f"e2e-crud-create-{generate_random_name()}" _, body = create_api_key( base_url=base_url, ocp_user_token=ocp_token_for_actor, request_session_http=request_session_http, api_key_name=key_name, ) + key_id = body["id"] + try: for field in ("id", "key", "name"): assert field in body, f"Expected '{field}' field in create response" key = body["key"] assert key.startswith("sk-oai-"), "Expected key to start with 'sk-oai-' prefix" assert len(key) > len("sk-oai-"), "Key body after prefix must not be empty" - LOGGER.info(f"[create] Created key id={body['id']}, key_prefix=sk-oai-***") + LOGGER.info(f"[create] Created key id={key_id}, key_prefix=sk-oai-***") get_resp, get_body = get_api_key( request_session_http=request_session_http, base_url=base_url, - key_id=body["id"], + key_id=key_id, ocp_user_token=ocp_token_for_actor, ) assert get_resp.status_code == 200, ( - f"Expected 200 on GET /v1/api-keys/{body['id']}, got {get_resp.status_code}: {get_resp.text[:200]}" + f"Expected 200 on GET /v1/api-keys/{key_id}, got {get_resp.status_code}" ) assert "key" not in get_body, "Plaintext key must not be returned by GET after creation (show-once pattern)" + finally: + revoke_api_key( + request_session_http=request_session_http, + base_url=base_url, + key_id=key_id, + ocp_user_token=ocp_token_for_actor, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` around lines 27 - 62, The test_create_api_key leaves a created API key active; wrap the create/get assertions in a try/finally and call revoke_api_key in the finally block to ensure cleanup; use the returned body["id"] from create_api_key and the same ocp_token_for_actor and request_session_http when calling revoke_api_key so the key is revoked regardless of assertion failures (refer to test_create_api_key, create_api_key, get_api_key, revoke_api_key to locate the relevant calls).
144-144:⚠️ Potential issue | 🟠 MajorCWE-532: Full response body in assertion message may leak sensitive fields.
{get_body}dumps the entire response object on failure. Past review flagged this; still unaddressed. Log only the specific field being checked.Proposed fix
- assert get_body.get("status") == "revoked", f"Expected status='revoked' on GET after revoke, got: {get_body}" + assert get_body.get("status") == "revoked", f"Expected status='revoked', got status={get_body.get('status')!r}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` at line 144, The assertion exposes the full response body via f"{get_body}" which may leak sensitive data; update the assertion in the test (around the check using get_body and the assert currently comparing status == "revoked") to avoid printing the whole response on failure—log only the specific field checked (e.g., include get_body.get("status") or the status value) or a minimal descriptive message referencing the "status" field instead of the entire get_body object.
94-95:⚠️ Potential issue | 🟠 MajorCWE-532: Full response object in assertion message may leak sensitive fields.
Dumping
{item}on assertion failure exposes the entire dict to CI logs. Past review flagged this; still unaddressed.Proposed fix
for item in items: - assert "key" not in item, f"Plaintext key must not appear in any list item: {item}" + assert "key" not in item, "Plaintext key must not appear in any list item"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` around lines 94 - 95, The assertion currently prints the full item dict on failure which may leak secrets; update the assertion in the loop over items so it does not interpolate the entire dict into the message. Replace f"Plaintext key must not appear in any list item: {item}" with a non-sensitive message (e.g., "Plaintext key must not appear in any list item") or include only a safe identifier (e.g., item.get("id") or the loop index) when available; target the assertion around the items loop that checks "key" not in item.
75-92:⚠️ Potential issue | 🟡 MinorEventual-consistency risk: no retry/polling before asserting list membership.
list_api_keysis called immediately after keys are created by the fixture. If the backend has indexing/replication delay, assertions at lines 88-92 become nondeterministic. Past review flagged this.Proposed fix using polling
+from timeout_sampler import TimeoutSampler @@ - list_resp, list_body = list_api_keys( - request_session_http=request_session_http, - base_url=base_url, - ocp_user_token=ocp_token_for_actor, - filters={"status": ["active"]}, - sort={"by": "created_at", "order": "desc"}, - pagination={"limit": 50, "offset": 0}, - ) - assert list_resp.status_code == 200, ( - f"Expected 200 on POST /v1/api-keys/search, got {list_resp.status_code}: {list_resp.text[:200]}" - ) - - items: list[dict] = list_body.get("items") or list_body.get("data") or [] - assert len(items) >= 2, f"Expected at least 2 active keys, got {len(items)}" - - key_ids = [item["id"] for item in items] - for created_id in two_active_api_key_ids: - assert created_id in key_ids, f"Created key id={created_id} not found in listed keys" + for sample in TimeoutSampler( + wait_timeout=60, + sleep=5, + func=list_api_keys, + request_session_http=request_session_http, + base_url=base_url, + ocp_user_token=ocp_token_for_actor, + filters={"status": ["active"]}, + sort={"by": "created_at", "order": "desc"}, + pagination={"limit": 50, "offset": 0}, + ): + list_resp, list_body = sample + if list_resp.status_code != 200: + continue + items = list_body.get("items") or list_body.get("data") or [] + key_ids = [item["id"] for item in items] + if all(cid in key_ids for cid in two_active_api_key_ids): + break + else: + pytest.fail(f"Created keys not visible in list within timeout") + + assert len(items) >= 2, f"Expected at least 2 active keys, got {len(items)}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` around lines 75 - 92, The test currently asserts created API key IDs (two_active_api_key_ids) are present immediately after calling list_api_keys, which is flaky under eventual consistency; modify the test to poll/retry list_api_keys until all expected ids appear or a timeout elapses: implement a short loop (e.g., poll every 0.5–1s up to a configurable timeout like 10–30s) that calls list_api_keys, extracts items (as the existing items: list[dict] = ... logic), checks that every id in two_active_api_key_ids is in the returned key_ids, and only then proceeds to the existing assertions; if the timeout is reached without seeing all ids, fail the test with a clear message. Use the existing list_api_keys function and two_active_api_key_ids variable names so the change is localized and easy to review.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 561-580: The fixture two_active_api_key_ids creates API keys via
create_api_key but never revokes them, causing test-state leakage; change the
fixture to a yield-style fixture that returns the list of created IDs and then,
in the teardown after the yield, iterate over the IDs and call revoke_api_key
(using the same base_url, ocp_token_for_actor, and request_session_http) to
revoke each key, and log any revoke errors; ensure the fixture keeps the same
signature and scope and handles exceptions during cleanup so all created keys
are attempted to be revoked.
- Around line 583-600: The fixture active_api_key_id creates an API key but
never cleans it up, leaking test state; convert active_api_key_id into a
generator-style pytest fixture that yields the created key ID and then deletes
the key in teardown using the same HTTP session and tokens (use
request_session_http, base_url, ocp_token_for_actor) — mirror the pattern used
in two_active_api_key_ids: call create_api_key to produce body["id"], yield that
id, and after the yield call the API key deletion helper (e.g., delete_api_key
or the equivalent teardown call) to remove the created key and handle/log any
deletion errors.
---
Duplicate comments:
In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py`:
- Around line 27-62: The test_create_api_key leaves a created API key active;
wrap the create/get assertions in a try/finally and call revoke_api_key in the
finally block to ensure cleanup; use the returned body["id"] from create_api_key
and the same ocp_token_for_actor and request_session_http when calling
revoke_api_key so the key is revoked regardless of assertion failures (refer to
test_create_api_key, create_api_key, get_api_key, revoke_api_key to locate the
relevant calls).
- Line 144: The assertion exposes the full response body via f"{get_body}" which
may leak sensitive data; update the assertion in the test (around the check
using get_body and the assert currently comparing status == "revoked") to avoid
printing the whole response on failure—log only the specific field checked
(e.g., include get_body.get("status") or the status value) or a minimal
descriptive message referencing the "status" field instead of the entire
get_body object.
- Around line 94-95: The assertion currently prints the full item dict on
failure which may leak secrets; update the assertion in the loop over items so
it does not interpolate the entire dict into the message. Replace f"Plaintext
key must not appear in any list item: {item}" with a non-sensitive message
(e.g., "Plaintext key must not appear in any list item") or include only a safe
identifier (e.g., item.get("id") or the loop index) when available; target the
assertion around the items loop that checks "key" not in item.
- Around line 75-92: The test currently asserts created API key IDs
(two_active_api_key_ids) are present immediately after calling list_api_keys,
which is flaky under eventual consistency; modify the test to poll/retry
list_api_keys until all expected ids appear or a timeout elapses: implement a
short loop (e.g., poll every 0.5–1s up to a configurable timeout like 10–30s)
that calls list_api_keys, extracts items (as the existing items: list[dict] =
... logic), checks that every id in two_active_api_key_ids is in the returned
key_ids, and only then proceeds to the existing assertions; if the timeout is
reached without seeing all ids, fail the test with a clear message. Use the
existing list_api_keys function and two_active_api_key_ids variable names so the
change is localized and easy to review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5cf48f20-e65a-485e-8244-8e4f9c89bd4e
📒 Files selected for processing (2)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
SB159
force-pushed
the
feature/api-key-crud-tests
branch
from
72086c0 to
8b1bf1e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py (2)
27-62:⚠️ Potential issue | 🟠 Major
test_create_api_keycreates an API key without cleanup (test-state leakage).This test creates an API key via
create_api_keybut does not use a fixture with teardown. The key persists after test completion, potentially causing quota issues or cross-test interference.Proposed fix: wrap in try/finally with revoke_api_key
+from tests.model_serving.maas_billing.maas_subscription.utils import revoke_api_key ... def test_create_api_key( self, request_session_http: requests.Session, base_url: str, ocp_token_for_actor: str, ) -> None: """Verify API key creation and show-once behavior.""" key_name = f"e2e-crud-create-{generate_random_name()}" _, body = create_api_key( base_url=base_url, ocp_user_token=ocp_token_for_actor, request_session_http=request_session_http, api_key_name=key_name, ) + key_id = body["id"] + try: for field in ("id", "key", "name"): assert field in body, f"Expected '{field}' field in create response" key = body["key"] assert key.startswith("sk-oai-"), "Expected key to start with 'sk-oai-' prefix" assert len(key) > len("sk-oai-"), "Key body after prefix must not be empty" LOGGER.info(f"[create] Created key id={body['id']}, key_prefix=sk-oai-***") get_resp, get_body = get_api_key( request_session_http=request_session_http, base_url=base_url, - key_id=body["id"], + key_id=key_id, ocp_user_token=ocp_token_for_actor, ) assert get_resp.status_code == 200, ( - f"Expected 200 on GET /v1/api-keys/{body['id']}, got {get_resp.status_code}: {get_resp.text[:200]}" + f"Expected 200 on GET /v1/api-keys/{key_id}, got {get_resp.status_code}: {get_resp.text[:200]}" ) assert "key" not in get_body, "Plaintext key must not be returned by GET after creation (show-once pattern)" + finally: + revoke_api_key( + request_session_http=request_session_http, + base_url=base_url, + key_id=key_id, + ocp_user_token=ocp_token_for_actor, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` around lines 27 - 62, test_create_api_key creates an API key via create_api_key and never revokes it; wrap the key creation and assertions in a try/finally that calls revoke_api_key(request_session_http=request_session_http, base_url=base_url, key_id=body["id"], ocp_user_token=ocp_token_for_actor) in the finally block so the created key is always removed even if assertions fail; ensure body is set before the finally (e.g., declare body = None then assign) and only call revoke_api_key when body and body["id"] exist.
94-95:⚠️ Potential issue | 🟠 MajorCWE-532: Assertion message exposes full
itemdict which may contain sensitive fields.On test failure, the assertion message dumps the entire item object to logs. Per past review, this was flagged but not addressed.
Proposed fix
for item in items: - assert "key" not in item, f"Plaintext key must not appear in any list item: {item}" + assert "key" not in item, "Plaintext key must not appear in any list item"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` around lines 94 - 95, The assertion currently exposes the full sensitive item dict in the failure message; update the assertion in the loop that iterates over items (the check using "items" and the assertion line with f"Plaintext key must not appear...") to remove the raw item dump—either use a generic message like "Plaintext key must not appear in any list item" or include only a non-sensitive identifier (e.g., item.get("id") or a masked value) instead of the full item dict so the test failure does not log sensitive fields.tests/model_serving/maas_billing/maas_subscription/conftest.py (2)
674-680:⚠️ Potential issue | 🟡 MinorTeardown lacks exception handling.
Same pattern issue:
revoke_api_keycan raiseAssertionError. Wrap in try/except to log and suppress cleanup failures.Proposed fix
yield body["id"] - LOGGER.info(f"Fixture teardown: revoking key {body['id']}") - revoke_api_key( - request_session_http=request_session_http, - base_url=base_url, - key_id=body["id"], - ocp_user_token=ocp_token_for_actor, - ) + try: + LOGGER.info(f"Fixture teardown: revoking key {body['id']}") + revoke_api_key( + request_session_http=request_session_http, + base_url=base_url, + key_id=body["id"], + ocp_user_token=ocp_token_for_actor, + ) + except Exception as exc: + LOGGER.warning(f"Cleanup failed for key {body['id']}: {exc}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 674 - 680, The fixture teardown calls revoke_api_key(...) which can raise AssertionError but is not handled; wrap the revoke_api_key invocation in a try/except that catches AssertionError (optionally Exception for safety), log the failure via LOGGER.error including the key id (body['id']) and the exception details, and suppress the exception so teardown continues; keep the existing LOGGER.info call and pass the same parameters (request_session_http, base_url, key_id=body["id"], ocp_user_token=ocp_token_for_actor) inside the try block.
646-653:⚠️ Potential issue | 🟡 MinorTeardown loop lacks exception handling; failure on first key leaks subsequent keys.
If
revoke_api_keyraises (e.g.,AssertionErroron non-JSON response perutils.py:303-306), the loop terminates and remaining keys are never revoked.Proposed fix
yield ids for key_id in ids: - LOGGER.info(f"Fixture teardown: revoking key {key_id}") - revoke_api_key( - request_session_http=request_session_http, - base_url=base_url, - key_id=key_id, - ocp_user_token=ocp_token_for_actor, - ) + try: + LOGGER.info(f"Fixture teardown: revoking key {key_id}") + revoke_api_key( + request_session_http=request_session_http, + base_url=base_url, + key_id=key_id, + ocp_user_token=ocp_token_for_actor, + ) + except Exception as exc: + LOGGER.warning(f"Cleanup failed for key {key_id}: {exc}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 646 - 653, The teardown loop over ids should not abort on the first revoke failure; wrap the call to revoke_api_key inside a try/except that catches Exception, logs an error with the key_id and exception (using LOGGER) and continues to the next id so all keys are attempted; ensure you still pass request_session_http, base_url and ocp_token_for_actor to revoke_api_key and consider logging a final summary if any revokes failed.
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py (1)
99-122: Test passes vacuously if no active keys exist.
assert len(paged_items) <= 1succeeds whenpaged_itemsis empty. This doesn't validate pagination logic—it only confirms the API didn't return more than requested. Consider assertinglen(paged_items) == 1after ensuring at least one key exists via fixture.Proposed fix: use fixture to guarantee keys exist
`@pytest.mark.tier1` `@pytest.mark.parametrize`("ocp_token_for_actor", [{"type": "admin"}], indirect=True) def test_list_api_keys_pagination( self, request_session_http: requests.Session, base_url: str, ocp_token_for_actor: str, + two_active_api_key_ids: list[str], # ensures at least 2 keys exist ) -> None: """Verify that the search endpoint respects the pagination limit.""" ... paged_items: list[dict] = page_body.get("items") or page_body.get("data") or [] - assert len(paged_items) <= 1, f"Expected at most 1 item with limit=1, got {len(paged_items)}" + assert len(paged_items) == 1, f"Expected exactly 1 item with limit=1, got {len(paged_items)}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py` around lines 99 - 122, The test test_list_api_keys_pagination can pass vacuously when no active API keys exist because it only asserts len(paged_items) <= 1; update the test to ensure at least one active key exists (use or add a fixture/helper that creates an active key via the same API used in list_api_keys) and then change the assertion to assert len(paged_items) == 1 to validate pagination behavior; reference list_api_keys and the paged_items variable when adding the setup fixture or call so the test reliably exercises the pagination logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 683-708: The teardown for active_api_key_id must tolerate
double-revocation to avoid teardown failures after revoked_api_key_id already
revoked it: update the active_api_key_id fixture's finalizer to call
revoke_api_key inside a try/except and treat expected outcomes (HTTP 200 with
status='revoked', HTTP 409 conflict, or a non-JSON/non-200 response indicating
"already revoked") as non-fatal; specifically, catch JSON decode errors and
request exceptions, log them at debug/info, and only raise/assert on unexpected
error codes or responses. Use the existing revoke_api_key helper and reference
the fixtures active_api_key_id and revoked_api_key_id so the teardown becomes
idempotent and safe against double revocation.
In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py`:
- Line 144: The assertion exposes the full response body via {get_body}; change
the message to avoid dumping sensitive data by only including non-sensitive
fields (e.g., the status) or a minimal identifier: update the assertion in
test_api_key_crud.py that reads assert get_body.get("status") == "revoked",
f"Expected status='revoked' on GET after revoke, got: {get_body}" to something
like asserting the same condition but using a message that references only
get_body.get("status") (for example: f"Expected status='revoked' on GET after
revoke, got status={get_body.get('status')}") so the test still reports useful
info without printing the entire response body.
---
Duplicate comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 674-680: The fixture teardown calls revoke_api_key(...) which can
raise AssertionError but is not handled; wrap the revoke_api_key invocation in a
try/except that catches AssertionError (optionally Exception for safety), log
the failure via LOGGER.error including the key id (body['id']) and the exception
details, and suppress the exception so teardown continues; keep the existing
LOGGER.info call and pass the same parameters (request_session_http, base_url,
key_id=body["id"], ocp_user_token=ocp_token_for_actor) inside the try block.
- Around line 646-653: The teardown loop over ids should not abort on the first
revoke failure; wrap the call to revoke_api_key inside a try/except that catches
Exception, logs an error with the key_id and exception (using LOGGER) and
continues to the next id so all keys are attempted; ensure you still pass
request_session_http, base_url and ocp_token_for_actor to revoke_api_key and
consider logging a final summary if any revokes failed.
In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py`:
- Around line 27-62: test_create_api_key creates an API key via create_api_key
and never revokes it; wrap the key creation and assertions in a try/finally that
calls revoke_api_key(request_session_http=request_session_http,
base_url=base_url, key_id=body["id"], ocp_user_token=ocp_token_for_actor) in the
finally block so the created key is always removed even if assertions fail;
ensure body is set before the finally (e.g., declare body = None then assign)
and only call revoke_api_key when body and body["id"] exist.
- Around line 94-95: The assertion currently exposes the full sensitive item
dict in the failure message; update the assertion in the loop that iterates over
items (the check using "items" and the assertion line with f"Plaintext key must
not appear...") to remove the raw item dump—either use a generic message like
"Plaintext key must not appear in any list item" or include only a non-sensitive
identifier (e.g., item.get("id") or a masked value) instead of the full item
dict so the test failure does not log sensitive fields.
---
Nitpick comments:
In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_crud.py`:
- Around line 99-122: The test test_list_api_keys_pagination can pass vacuously
when no active API keys exist because it only asserts len(paged_items) <= 1;
update the test to ensure at least one active key exists (use or add a
fixture/helper that creates an active key via the same API used in
list_api_keys) and then change the assertion to assert len(paged_items) == 1 to
validate pagination behavior; reference list_api_keys and the paged_items
variable when adding the setup fixture or call so the test reliably exercises
the pagination logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 66f5f173-4c30-48a5-b33a-a54368d2ccfc
📒 Files selected for processing (3)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_api_key_crud.pytests/model_serving/maas_billing/maas_subscription/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_serving/maas_billing/maas_subscription/utils.py
dbasunag
reviewed
Mar 18, 2026
dbasunag
approved these changes
Mar 18, 2026
|
Status of building tag latest: success. |
ssaleem-rh
pushed a commit
to ssaleem-rh/opendatahub-tests
that referenced
this pull request
Mar 23, 2026
* test: add MaaS API key tests Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * address coderabit review comments Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * address coderabit comments Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * address review comment Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * address latest coderabit review comments Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> --------- Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Signed-off-by: Shehan Saleem <ssaleem@redhat.com>
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
4 participants
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.
Pull Request
Summary
add MaaS API key tests
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit