Uh oh!
There was an error while loading. Please reload this page.
test(admin): apikey rotate auth-required + concurrent race atomicity - #374
Conversation
Adds 2 unit tests in crates/aisix-admin/src/lib.rs covering the gaps the audit on api7/AISIX-Cloud#398 Addendum.B flagged for "POST /admin/v1/apikeys/:id/rotate": ✓ rotate_apikey_requires_admin_auth — request without Bearer admin-secret header returns 401 BEFORE touching the store. Pins this so a future "accidental open endpoint" refactor can't ship undetected. ✓ concurrent_rotate_apikey_serializes_atomically — fires two rotations against the same key concurrently via tokio::spawn; both must succeed with distinct revisions, the final stored hash matches the higher-revision winner, and the loser's plaintext must NOT match the final hash. Catches the race-window admit ("new + old both work") that the audit explicitly named. Atomicity comes from the in-memory store's RwLock semantics; a future refactor to an etcd-backed store without CAS would regress the concurrent test. Three tests now run in the rotate_apikey family: ✓ rotate_apikey_generates_new_key_and_increments_revision ✓ rotate_apikey_requires_admin_auth (NEW) ✓ concurrent_rotate_apikey_serializes_atomically (NEW) All pass: `cargo test -p aisix-admin --lib rotate_apikey` → 3 passed; 0 failed. Tracking: api7/AISIX-Cloud#398 Tier 2 v2-HIGH "POST /admin/v1/ apikeys/:id/rotate".
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds two integration tests for the API key rotation endpoint in the admin module. The first test validates authentication enforcement, ensuring unauthenticated rotation requests return 401. The second test verifies concurrency handling by spawning simultaneous rotate requests against the same key and confirming atomic serialization with monotonic revision progression. ChangesAPI Key Rotation Integration Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
There was a problem hiding this comment.
Pull request overview
Adds targeted unit test coverage in aisix-admin for POST /admin/v1/apikeys/:id/rotate, specifically around (1) enforcing admin authentication and (2) behavior under concurrent rotation requests—addressing concerns raised in api7/AISIX-Cloud#398 Addendum.B.
Changes:
- Add a unit test asserting
/rotatereturns401 Unauthorizedwhen called without admin auth. - Add a unit test that fires two concurrent
/rotatecalls and asserts distinct returned plaintexts/revisions and that the final stored hash matches the “winner”.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This pins the rotation race-window concern from #398 | ||
| // Addendum.B: "rotation race window (new + old both admit | ||
| // simultaneously)". Atomicity comes from the store's RwLock; a | ||
| // refactor to an etcd-backed store without CAS semantics would | ||
| // regress this test. |
| // Fire two rotations concurrently. Each rotation is one | ||
| // PUT to the in-memory ConfigStore, so the RwLock serializes | ||
| // them; both should succeed with monotonically-increasing | ||
| // revisions. | ||
| let app_a = build_router(state.clone()); | ||
| let app_b = build_router(state.clone()); | ||
| let id_a = id.clone(); | ||
| let id_b = id.clone(); | ||
| let task_a = tokio::spawn(async move { | ||
| run( | ||
| app_a, | ||
| auth_req("POST", &format!("/admin/v1/apikeys/{id_a}/rotate"), None), | ||
| ) | ||
| .await | ||
| }); | ||
| let task_b = tokio::spawn(async move { |
| // Create the key with auth so we have a valid id to target. | ||
| let app = build_router(state.clone()); | ||
| let resp = run( | ||
| app, | ||
| auth_req( | ||
| "POST", | ||
| "/admin/v1/apikeys", | ||
| Some(apikey_payload("sk-original", &["my-model"])), | ||
| ), | ||
| ) | ||
| .await; | ||
| let id = body_json(resp).await["id"].as_str().unwrap().to_string(); | ||
| // Now hit /rotate WITHOUT the admin Bearer header. | ||
| let app = build_router(state); | ||
| let req = Request::builder() | ||
| .method("POST") | ||
| .uri(format!("/admin/v1/apikeys/{id}/rotate")) | ||
| .body(Body::empty()) | ||
| .unwrap(); | ||
| let resp = run(app, req).await; | ||
| assert_eq!( | ||
| resp.status(), | ||
| StatusCode::UNAUTHORIZED, | ||
| "rotate must require admin auth — unauthenticated callers must NOT be able to invalidate or replace an api_key", | ||
| ); |
Summary
Adds 2 unit tests in
crates/aisix-admin/src/lib.rs:rotate_apikey_requires_admin_auth— unauthenticated request returns 401 before touching the storeconcurrent_rotate_apikey_serializes_atomically— two concurrent rotations both succeed with distinct revisions; final stored hash matches the winner~140 LOC, single test file extended.
Why
The audit on api7/AISIX-Cloud#398 Addendum.B explicitly listed
POST /admin/v1/apikeys/:id/rotateas a HIGH-severity surface — "Rotation race window (new + old both admit simultaneously) has no test" and an auth-bypass surface. Existing tests in this file cover the happy-path single rotation + 404-on-missing, but not the auth-required or race-window cases.This PR closes both gaps via in-process tokio-spawn-based concurrency tests against the in-memory store.
Empirical run
Tracking
api7/AISIX-Cloud#398 Tier 2 v2-HIGH "POST /admin/v1/apikeys/:id/rotate e2e".
Summary by CodeRabbit