ci(authenticator): endpoint coverage gate + OpenAPI drift gate - #2000
Conversation
Give the authenticator the same endpoint-coverage machinery the
analytics/identity lanes have (e2e-bronze-to-api.yml):
- `authenticator openapi` subcommand: emit the OpenAPI document offline
from the exact build_operations route table the live gear serves
(mirrors `analytics openapi`); declare the provable 400 on
back-channel-logout while at it. Commit the generated doc at
docs/components/backend/authenticator/openapi.json and drift-check it
in openapi-specs.yml.
- tests/common/mod.rs: a reqwest wrapper the eight e2e_*.rs suites now
share; it records every (method, path) -> {status} answered by the
authenticator instances into $E2E_COVERAGE_LEDGER (same schema as the
bronze-to-api rig's observed_endpoints.json), filtered by origin so
fakeidp/service-token traffic can't pollute the ledger. run-e2e.sh
resets + exports the ledger path. The login loop now also asserts the
discovery document (the one operation no test exercised).
- api_coverage.py: an `authenticator` suite — one SKIP (admin
revoke-by-user needs the gateway authn pipeline; covered in the
gateway compose e2e), back-channel-logout's 200 BLOCKED (answered to
the IdP's server-side POST, proven via fakeidp's rp_status assertion;
the client only sees the 400 rejection). Genericize the BLOCKED
report preamble that was analytics-only prose.
- .github/workflows/authenticator.yml: e2e job runs run-e2e.sh (which
now didn't run in CI at all) and uploads the ledger;
authenticator-endpoint-coverage-gate analyses it against the
committed spec — a documented operation exercised by no test fails.
Local validation: run-e2e.sh green end-to-end; gate PASS (13/14
exercised + 1 approved SKIP, 0 missing); gate correctly FAILS (rc 1,
MISSING) when an operation's traffic is removed from the ledger.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
📝 WalkthroughWalkthroughAuthenticator routes now generate an offline OpenAPI document, e2e tests record endpoint coverage, and CI validates both specification drift and exercised operations against the committed Authenticator API specification. ChangesAuthenticator contract and CI gates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Authenticator
participant OpenAPI
participant CoverageGate
CI->>Authenticator: run offline openapi command
Authenticator-->>OpenAPI: emit generated JSON
CI->>Authenticator: run e2e suite
Authenticator-->>CoverageGate: upload observed endpoint ledger
CoverageGate->>OpenAPI: compare observed operations with committed spec
CoverageGate-->>CI: pass or fail coverage gate
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/services/authenticator/src/main.rs (1)
98-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefer
AppConfigloading until config is actually needed.
Commands::Openapisays no config is required, butmain()callsAppConfig::load_or_default(...)?,apply_cli_overrides(...), andprint_config’sto_yaml()path before dispatch, so the OpenAPI subcommand still performs config loading. If config validation defaults or errors on missing backend fields, CI drift-gate runs without a real config could be misclassified as config failures. Move config loading/printing into the run/check/print-config paths, or make the OpenAPI path use a no-config state.🤖 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/src/main.rs` around lines 98 - 118, Update main so command dispatch occurs before AppConfig::load_or_default and config.apply_cli_overrides, allowing Commands::Openapi to call print_openapi without loading or validating configuration. Keep configuration loading for Commands::Run and Commands::Check, and preserve the --print-config path only where a loaded config is available.
🧹 Nitpick comments (2)
.github/workflows/openapi-specs.yml (1)
59-70: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMissing
persist-credentials: falseon new checkout step.The new authenticator job's
actions/checkout@v4(Line 65) doesn't setpersist-credentials: false, unlike the checkout steps added in.github/workflows/authenticator.ymlin this same PR, which explicitly disable credential persistence. Static analysis flags this (artipacked). Align for consistency.🔒 Proposed fix
- uses: actions/checkout@v4 + with: + persist-credentials: false - name: Install protoc + librdkafka build deps🤖 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 @.github/workflows/openapi-specs.yml around lines 59 - 70, Update the authenticator job’s actions/checkout@v4 step to set persist-credentials: false, matching the existing checkout configuration in authenticator.yml.Source: Linters/SAST tools
docs/components/backend/authenticator/openapi.json (1)
1-435: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueCookie-authenticated endpoints have no declared
security.Only the admin
bearerAuthendpoint (Lines 132-136) declares asecurityrequirement; every cookie-session-protected route (/auth/me,/auth/csrf,/auth/refresh,/auth/sessions*, etc.) declares none. Static analysis (checkov CKV_OPENAPI_4/5) flags this as no global/operation-level security. Since this doc is generated fromOperationBuilderregistrations, the underlying gap (if real) would need to be fixed inapi/mod.rs/thetoolkitOperationBuilder, not here directly. Given this appears to be a modeling limitation (cookie auth isn't representable as abearerAuth-style scheme) rather than a missing enforcement, this is likely fine to leave as-is, but worth confirming whethertoolkit::api::OperationBuildersupports declaring a cookie-based security scheme for documentation completeness.🤖 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 `@docs/components/backend/authenticator/openapi.json` around lines 1 - 435, Confirm whether toolkit::api::OperationBuilder supports declaring cookie-based security schemes; if supported, add the appropriate scheme and apply it to cookie-authenticated operations such as authenticator.me, authenticator.csrf, authenticator.refresh, and authenticator.sessions.* in the registrations under api/mod.rs, then regenerate the OpenAPI document. If unsupported, leave enforcement unchanged and document or otherwise preserve this modeling limitation rather than editing the generated JSON directly.Source: Linters/SAST tools
🤖 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_login_loop.rs`:
- Around line 184-203: Update the discovery assertions in the e2e login test to
extract the advertised jwks_uri, perform an HTTP GET against it, and parse the
response as JWKS; validate that the fetched key set contains the key material
required to verify the issued token, rather than only checking the URL suffix.
Preserve the existing issuer assertion and use the test’s existing token/JWKS
verification symbols where available.
---
Outside diff comments:
In `@src/backend/services/authenticator/src/main.rs`:
- Around line 98-118: Update main so command dispatch occurs before
AppConfig::load_or_default and config.apply_cli_overrides, allowing
Commands::Openapi to call print_openapi without loading or validating
configuration. Keep configuration loading for Commands::Run and Commands::Check,
and preserve the --print-config path only where a loaded config is available.
---
Nitpick comments:
In @.github/workflows/openapi-specs.yml:
- Around line 59-70: Update the authenticator job’s actions/checkout@v4 step to
set persist-credentials: false, matching the existing checkout configuration in
authenticator.yml.
In `@docs/components/backend/authenticator/openapi.json`:
- Around line 1-435: Confirm whether toolkit::api::OperationBuilder supports
declaring cookie-based security schemes; if supported, add the appropriate
scheme and apply it to cookie-authenticated operations such as authenticator.me,
authenticator.csrf, authenticator.refresh, and authenticator.sessions.* in the
registrations under api/mod.rs, then regenerate the OpenAPI document. If
unsupported, leave enforcement unchanged and document or otherwise preserve this
modeling limitation rather than editing the generated JSON directly.
🪄 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: e898594b-ff18-4bc0-ad11-0b1f036e210b
📒 Files selected for processing (17)
.github/workflows/authenticator.yml.github/workflows/openapi-specs.ymldocs/components/backend/authenticator/openapi.jsonsrc/backend/services/authenticator/src/api/mod.rssrc/backend/services/authenticator/src/main.rssrc/backend/services/authenticator/tests/.gitignoresrc/backend/services/authenticator/tests/common/mod.rssrc/backend/services/authenticator/tests/e2e_backchannel.rssrc/backend/services/authenticator/tests/e2e_login_loop.rssrc/backend/services/authenticator/tests/e2e_override.rssrc/backend/services/authenticator/tests/e2e_ratelimit.rssrc/backend/services/authenticator/tests/e2e_refresh.rssrc/backend/services/authenticator/tests/e2e_refresher.rssrc/backend/services/authenticator/tests/e2e_service_token.rssrc/backend/services/authenticator/tests/e2e_sessions.rssrc/backend/services/authenticator/tests/run-e2e.shsrc/ingestion/tests/e2e/lib/api_coverage.py
| // 5b. The discovery document points downstream verifiers at that JWKS | ||
| // (cf-gears-oidc-authn-plugin resolves jwks_uri from it). | ||
| let discovery: serde_json::Value = http | ||
| .get(format!("{auth_base}/.well-known/openid-configuration")) | ||
| .send() | ||
| .await | ||
| .unwrap() | ||
| .json() | ||
| .await | ||
| .unwrap(); | ||
| assert!( | ||
| discovery["issuer"].as_str().is_some_and(|s| !s.is_empty()), | ||
| "discovery must carry the issuer" | ||
| ); | ||
| assert!( | ||
| discovery["jwks_uri"] | ||
| .as_str() | ||
| .is_some_and(|s| s.ends_with("/.well-known/jwks.json")), | ||
| "discovery must point at the published JWKS" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Dereference the discovered jwks_uri.
The suffix check accepts an unrelated or dead URL. Fetch jwks_uri and verify it serves the JWKS used to validate the issued token, so this actually tests the discovery contract.
🤖 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_login_loop.rs` around lines 184
- 203, Update the discovery assertions in the e2e login test to extract the
advertised jwks_uri, perform an HTTP GET against it, and parse the response as
JWKS; validate that the fetched key set contains the key material required to
verify the issued token, rather than only checking the URL suffix. Preserve the
existing issuer assertion and use the test’s existing token/JWKS verification
symbols where available.
2 participants
Gives the authenticator the same endpoint-coverage machinery the analytics/identity lanes have (
e2e-bronze-to-api.yml): a committed OpenAPI spec kept fresh by a drift gate, an e2e ledger of every request the suite makes, and a CI gate that fails when a documented operation is exercised by no test.What's in here
authenticator openapisubcommand — emits the OpenAPI document offline from the exactbuild_operationsroute table the live gear serves (mirrorsanalytics openapi), so the committed doc and the running service can never diverge. Declares the provable400on back-channel-logout while at it (the e2e asserts it). Committed doc:docs/components/backend/authenticator/openapi.json(14 operations); drift-checked by a new job inopenapi-specs.yml.tests/common/mod.rs, a thin reqwest wrapper all 8e2e_*.rssuites now share. It records(method, path) -> {status}into$E2E_COVERAGE_LEDGER(same schema as the bronze-to-api rig'sobserved_endpoints.json), origin-filtered byAUTH_BASE/AUTH_BASE_DISABLEDso fakeidp / service-token-listener traffic can't pollute the ledger.run-e2e.shresets + exports the path. The login loop now also asserts the discovery document — the one operation no test exercised.authenticatorsuite inapi_coverage.py— one SKIP (DELETE /auth/admin/users/{person_id}/sessionsneeds the gateway-JWT authn pipeline; exercised in the gateway compose e2e) and one BLOCKED code (back-channel-logout's200is answered to the IdP's server-side POST — proven via fakeidp'srp_statusassertion — the test client can only observe the400rejection). Also genericizes the BLOCKED report preamble that was analytics-only prose..github/workflows/authenticator.yml— ane2ejob that runsrun-e2e.sh(this suite previously didn't run in CI at all) and uploads the ledger, plus anauthenticator-endpoint-coverage-gatejob that analyses it against the committed spec. Path-filtered likegateway.yml.Gate state
PASS— 13/14 operations exercised + 1 approved SKIP, 0 missing. Non-blocking advisories:401unobserved on the csrf/sessions ops; observed-but-undeclared403(CSRF reject) and429(rate limiter) — follow-up candidates for.error_401/.error_403/.error_429declarations.Validation
run-e2e.shgreen locally with recording on; gatePASSon the real ledger.MISSING(rc 1).openapi_spec.py checkgreen against the committed doc; unit tests (50), clippy, rustfmt, pre-commit all pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests