test: add MaaS API key bulk revoke tests#1282
Merged
dbasunag merged 4 commits intoopendatahub-io:mainfrom Mar 24, 2026
Merged
Conversation
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/wip', '/build-push-pr-image', '/hold', '/verified', '/lgtm'} |
for more information, see https://pre-commit.ci
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a function-scoped pytest fixture that creates three active MaaS API keys and revokes them on teardown; adds two helper functions to perform and assert bulk API-key revocations; and adds a new test module with three Tier1 tests covering self, forbidden cross-user, and admin cross-user bulk-revoke scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security flagging
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.py (1)
88-111: Missing post-revoke verification.Unlike
test_bulk_revoke_own_keys, this test doesn't verify the key's status is actually"revoked"after the bulk operation. Consider adding aget_api_keycheck to confirm theactive_api_key_idkey hasstatus == "revoked".Proposed addition
# After the assert for revoked_count, add: get_resp, get_body = get_api_key( request_session_http=request_session_http, base_url=base_url, key_id=active_api_key_id, ocp_user_token=admin_ocp_token, ) assert get_resp.status_code == 200, ( f"Expected 200 on GET /v1/api-keys/{active_api_key_id}, got {get_resp.status_code}" ) assert get_body.get("status") == "revoked", ( f"Expected key id={active_api_key_id} to have status='revoked', got: {get_body.get('status')}" )🤖 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_bulk_operations.py` around lines 88 - 111, The test test_bulk_revoke_admin_can_revoke_any_user currently asserts revokedCount but doesn't verify the actual key status; after the revoked_count assert, call get_api_key with request_session_http, base_url, key_id=active_api_key_id and ocp_user_token=admin_ocp_token, assert the GET response status_code is 200 and that the returned body has "status" == "revoked" to confirm the key (active_api_key_id) was actually revoked; use the existing helper get_api_key and keep this check immediately after the revoked_count assertion.
🤖 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 678-685: The teardown loop calling revoke_api_key for each key_id
can fail if keys were already bulk-revoked (non-2xx/404 responses); update the
teardown in the three_active_api_key_ids fixture to handle and ignore
404/"already revoked" responses the same way create_and_yield_api_key_id does by
catching the API response/exception from revoke_api_key and treating 404 or an
"already revoked" error as success (log and continue) while still raising on
other errors.
In
`@tests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.py`:
- Around line 54-65: The loop over three_active_api_key_ids silently skips
verification when get_api_key returns non-200; update the loop that calls
get_api_key (using request_session_http, base_url, ocp_token_for_actor) to
assert the response is 200 (e.g., assert get_resp.status_code == 200 with a
helpful message including key_id and get_resp.status_code) before checking
get_body.get("status") == "revoked", and log or fail explicitly if the response
is not 200 so deletions or missing keys aren't masked; keep the final
LOGGER.info after successful confirmations.
---
Nitpick comments:
In
`@tests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.py`:
- Around line 88-111: The test test_bulk_revoke_admin_can_revoke_any_user
currently asserts revokedCount but doesn't verify the actual key status; after
the revoked_count assert, call get_api_key with request_session_http, base_url,
key_id=active_api_key_id and ocp_user_token=admin_ocp_token, assert the GET
response status_code is 200 and that the returned body has "status" == "revoked"
to confirm the key (active_api_key_id) was actually revoked; use the existing
helper get_api_key and keep this check immediately after the revoked_count
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3a179567-65c1-42c4-8b9c-70d8bdb1a222
📒 Files selected for processing (3)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.pytests/model_serving/maas_billing/maas_subscription/utils.py
tests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.py
Show resolved
Hide resolved
dbasunag
reviewed
Mar 24, 2026
tests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.py
Show resolved
Hide resolved
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
rhods-ci-bot
added
commented-by-SB159
and removed
commented-by-dbasunag
commented-by-SB159
labels
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_bulk_operations.py`:
- Around line 25-33: The test_bulk_revoke_own_keys test currently forces an
admin principal by passing ocp_token_for_actor={"type": "admin"}, which
validates admin behavior not self-service; change that param to use a non-admin
principal (e.g., ocp_token_for_actor={"type": "user"} or another non-admin role
your fixture supports) so the test exercises ordinary-user self-service bulk
revoke paths and update any related docstring if needed.
- Around line 102-118: The test currently only checks revoked_count from
assert_bulk_revoke_success but never verifies the specific active_api_key_id was
revoked; after calling assert_bulk_revoke_success (or wherever revoked_count is
returned), call get_api_key(request_session_http=..., base_url=...,
ocp_user_token=admin_ocp_token, username=free_user_username,
api_key_id=active_api_key_id) and assert the returned key's status == "revoked"
(or equivalent field), failing the test if it is not; reference
active_api_key_id, assert_bulk_revoke_success (or bulk_revoke_api_keys if
reverted), and get_api_key to locate where to add this assertion.
- Around line 49-56: Resolve the merge markers and pick one consistent
implementation: prefer the bulk_revoke_api_keys() style that returns (bulk_resp,
bulk_body), remove all <<<<>>>> markers, restore the HEAD assertions that check
bulk_resp.status_code == 200 and compute revoked_count =
bulk_body.get("revokedCount", 0), and update any callers that were changed to
expect a single revoked_count (e.g., replace calls to
assert_bulk_revoke_success() with the tuple-returning bulk_revoke_api_keys()
usage or adapt their logic to read revoked_count from bulk_body); ensure
revoked_count is defined before it is referenced and that function signatures
for bulk_revoke_api_keys and any helper assert_* are consistent across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 39e76174-7c2a-4737-93a2-7ae4b9f18949
📒 Files selected for processing (3)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.pytests/model_serving/maas_billing/maas_subscription/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/model_serving/maas_billing/maas_subscription/conftest.py
- tests/model_serving/maas_billing/maas_subscription/utils.py
tests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.py
Outdated
Show resolved
Hide resolved
tests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.py
Outdated
Show resolved
Hide resolved
tests/model_serving/maas_billing/maas_subscription/test_api_key_bulk_operations.py
Show resolved
Hide resolved
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
dbasunag
approved these changes
Mar 24, 2026
|
Status of building tag latest: success. |
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
3 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 bulk revoke tests
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit