test(authenticator): e2e 401 contract for the session-cookie surface - #2131
Conversation
Cover the declared-but-untested 401 responses on GET /auth/me,
GET /auth/csrf, GET /auth/sessions, DELETE /auth/sessions, and
DELETE /auth/sessions/{session_id}: each must answer 401 both without
a cookie and with a token that resolves to no session. Raises the
authenticator endpoint-coverage gate's registered-code coverage to
21/24; the remaining gaps are the skip-listed admin revoke route.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds E2E coverage for missing, invalid, and unknown authentication credentials. It validates HTTP 401 responses for authenticator session routes and ingestion API operations. It also updates coverage rules and documentation to require observed 401 responses. ChangesUnauthorized authentication coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…rom boilerplate The e2e rig runs auth-ENABLED (the gears host's oidc-authn-plugin verifies the gateway JWT), so the UNIVERSAL_BOILERPLATE exclusion of 401 dated from the retired auth-disabled rig and was hiding a real, testable contract. Keep only 429 (no rate limiter) excluded, and observe 401 on all 29 spec operations via an anonymous recording client (api/test_unauthorized.py), plus a malformed-bearer case. Registered-code coverage: 97/104 (93.3%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/ingestion/tests/e2e/api/test_unauthorized.py (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the module docstring.
The docstring restates the test behavior and coverage implementation. Keep this rationale in tests or design documentation instead.
As per coding guidelines, “Do not add module docstring headers that restate code, issue numbers, or phase/scope notes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tests/e2e/api/test_unauthorized.py` around lines 1 - 8, Remove the module-level docstring at the beginning of the unauthorized API test module; leave the test implementations and coverage behavior unchanged.Source: Coding guidelines
src/backend/services/authenticator/tests/e2e_unauthorized.rs (2)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the module documentation block.
This service test is not an exported shared-library item. The block restates test behavior and adds runner instructions.
As per coding guidelines, “Use
///documentation comments only on exported items in shared library crates” and “Do not add documentation comments to binaries or services.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/tests/e2e_unauthorized.rs` around lines 1 - 14, Remove the module-level `//!` documentation block at the top of the unauthorized end-to-end test; leave the test implementation unchanged.Source: Coding guidelines
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the crate-wide
allowwith a justifiedexpect.
#![allow(...)]can hide future unrelated violations. Use a narrow#[expect(clippy::...)]or crate-level#![expect(...)]with a same-line reason.As per coding guidelines, “Prefer
#[expect(clippy::...)]over#[allow]; provide the justification on the same line as either lint attribute.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/tests/e2e_unauthorized.rs` at line 16, Replace the crate-wide `#![allow(clippy::unwrap_used, clippy::expect_used)]` in the unauthorized E2E test with a narrow `#[expect]` or justified crate-level `#![expect]`, including the reason on the same line; scope each expectation only to the intentional unwrap/expect usage rather than suppressing future violations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/services/authenticator/tests/e2e_unauthorized.rs`:
- Around line 70-85: Update the unauthorized session-token test loop around
SESSION_ROUTES to read the response body with resp.text().await and assert it
matches the {"error":"unauthenticated"} payload already verified by
session_routes_reject_missing_cookie_with_401, while preserving the existing 401
status assertion.
In `@src/ingestion/tests/e2e/api/conftest.py`:
- Around line 30-38: Update the anon_api fixture signature to include an
explicit generator/iterator return annotation for the yielded httpx.Client,
using the imported HTTPX type and the project’s preferred annotation style.
---
Nitpick comments:
In `@src/backend/services/authenticator/tests/e2e_unauthorized.rs`:
- Around line 1-14: Remove the module-level `//!` documentation block at the top
of the unauthorized end-to-end test; leave the test implementation unchanged.
- Line 16: Replace the crate-wide `#![allow(clippy::unwrap_used,
clippy::expect_used)]` in the unauthorized E2E test with a narrow `#[expect]` or
justified crate-level `#![expect]`, including the reason on the same line; scope
each expectation only to the intentional unwrap/expect usage rather than
suppressing future violations.
In `@src/ingestion/tests/e2e/api/test_unauthorized.py`:
- Around line 1-8: Remove the module-level docstring at the beginning of the
unauthorized API test module; leave the test implementations and coverage
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d917d8b0-fcfd-433b-a039-d945099b8185
📒 Files selected for processing (6)
src/backend/services/authenticator/tests/e2e_unauthorized.rssrc/backend/services/authenticator/tests/run-e2e.shsrc/ingestion/tests/e2e/README.mdsrc/ingestion/tests/e2e/api/conftest.pysrc/ingestion/tests/e2e/api/test_unauthorized.pysrc/ingestion/tests/e2e/lib/api_coverage.py
…non_api fixture Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
2 participants
Summary
API-coverage pass for 401 responses: every endpoint that declares 401 in a committed OpenAPI spec should have a test observing it.
anon_api401 tests, blocking viaREQUIRED_EXTRA); no gaps.UNIVERSAL_BOILERPLATE, a leftover from the retired auth-disabled rig. The rig runs auth-ENABLED (the gears host's oidc-authn-plugin verifies the gateway JWT), so 401 is a real, testable contract — fixed here.GET /auth/me,GET /auth/csrf,GET /auth/sessions,DELETE /auth/sessions,DELETE /auth/sessions/{session_id}.authenticator
New
tests/e2e_unauthorized.rscovering the five routes: each must answer 401 with the{"error":"unauthenticated"}body without a cookie, and 401 with a cookie that resolves to no session. The CSRF middleware only fires once a session resolves, so the DELETEs genuinely exercise the 401 path. Wired intorun-e2e.sh; the ledger records the codes through the existing recording client. Registered-code coverage rises to 21/24 (87.5%) — the remaining gaps are the skip-listed admin revoke route (needs the gateway-JWT pipeline, exercised in the gateway compose e2e).analytics
UNIVERSAL_BOILERPLATEdrops to{429}(only the rate limiter remains unobservable); stale "auth disabled" wording corrected in theBLOCKEDtable and e2e README. Newapi/test_unauthorized.pyobserves 401 on all 29 spec operations via an anonymous recording client (newanon_apifixture, mirroring the identity suite), plus a malformed-bearer case. Registered-code coverage: 97/104 (93.3%).Note:
docs/domain/bronze-to-api-e2e/specs/(PRD/DESIGN/FEATURE) still describes the rig as auth-disabled — left untouched as spec artifacts; the drift deserves its own follow-up.Test plan
run-e2e.shfull stack green; coverage gate PASS, all five 401s observed./e2e.sh test api/green on a rebuilt runner image (124 passed, 14 xfailed);./e2e.sh gates apiPASS — 29/29 ops, 401 observed on every operationcargo fmt/clippy/pre-commitclean🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation