Wave 3b: auth refactor + trust boundary + trace classification - #10
Merged
Merged
Conversation
…ueprints#199) * fix auth trust boundary and enforce async job ownership * fix old tests * share request identity resolution for HTTP and WebSockets * remove log and fix no auth flow * make rollback async * skip ownership check when require auth is false
…prints#203) * add request trace classification and pseudonymous ids Signed-off-by: Ajay Thorve <athorve@nvidia.com> * harden trace user tagging guards Signed-off-by: Ajay Thorve <athorve@nvidia.com> * document trace env examples Signed-off-by: Ajay Thorve <athorve@nvidia.com> * extract auth trace helpers into utils Signed-off-by: Ajay Thorve <athorve@nvidia.com> * restrict access channel overrides Signed-off-by: Ajay Thorve <athorve@nvidia.com> --------- Signed-off-by: Ajay Thorve <athorve@nvidia.com>
* fix aiq request tags on nat spans Signed-off-by: Ajay Thorve <athorve@nvidia.com> * docs: clarify nat span request tagging Signed-off-by: Ajay Thorve <athorve@nvidia.com> * fix websocket request trace tags Signed-off-by: Ajay Thorve <athorve@nvidia.com> * generalize request trace access channels Signed-off-by: Ajay Thorve <athorve@nvidia.com> --------- Signed-off-by: Ajay Thorve <athorve@nvidia.com>
…or semantics (NVIDIA-AI-Blueprints#194) * fix: eliminate dual token refresh race and increase refresh buffer Two independent refresh mechanisms (SessionProvider.refetchInterval and a setInterval in useAuth) fired concurrently every ~4 minutes, causing "invalid_grant" failures with OAuth providers that use rotating refresh tokens (e.g., NVIDIA Starfleet SSO). The second concurrent refresh consumed an already-invalidated token and killed the session. Remove the duplicate setInterval from useAuth — session refresh is now handled solely by SessionProvider's refetchInterval. Make the interval config-driven (from TOKEN_REFRESH_BUFFER_SECONDS) instead of hardcoded. Also increase the default TOKEN_REFRESH_BUFFER_MINUTES from 5 to 15. The previous 5-minute window was insufficient for deployments running long operations (deep research with ECI runs 20-40+ minutes). Enterprise deployments should set TOKEN_REFRESH_BUFFER_MINUTES=30 via env var. Includes a regression test that verifies useAuth never creates its own setInterval, preventing re-introduction of the race condition. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-AI-Blueprints#195) * feat: add provider lifecycle hooks for composable auth extensions Add onSignIn and onSession lifecycle hooks to AuthProviderConfig, enabling auth providers to inject custom behavior (e.g. DL group gating, custom claims) without replacing the entire config.ts file. This reduces the overlay surface area for internal deployments — the Starfleet overlay can now provide hooks via its provider file instead of replacing config.ts and session.ts wholesale. The overlay goes from 4 replaced files to 2 (types.ts + providers/index.ts). New AuthProviderConfig fields: - onSignIn: async hook called after OAuth callback, return value merged into JWT (e.g. check group membership, add custom claims) - onSession: sync hook called on every session check, return value merged into session (e.g. surface hasAccess, groupName to client) - tokenRefreshBufferSeconds: provider-level override for the refresh buffer (takes precedence over TOKEN_REFRESH_BUFFER_MINUTES env var) - requiredEnvVars: additional env vars checked by validateAuthEnv() All hooks are optional — existing providers work unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve strict TypeScript errors in lifecycle hook code - Spread NextAuth Account/User/JWT objects before passing to hooks so they satisfy Record<string, unknown> (interface types lack index sigs) - Cast session callback results through unknown for Record access in tests (Session | DefaultSession lacks provider-specific fields) - Fix AdapterUser requiring email field in test mocks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: match all auth error codes in frontend RUM tracking Addresses Greptile P1: the WebSocket error handler only checked for message === "auth_error" but the backend now sends exc.error_code ("token_expired", "token_invalid"). Also applies severity split — expected auth events route to RUM actions, unexpected to RUM errors. - rum.ts: add trackRumAction, trackAuthEvent with expected/unexpected routing via EXPECTED_AUTH_CODES set - websocket-client: match against full set of auth error codes - authenticated-fetch: use trackAuthEvent for severity routing - session: use trackRumAction for expected refresh failures Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: preserve core auth fields in provider hooks * fix: address auth provider hook review comments * fix: guard auth provider sign-in hook --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Ajay Thorve <athorve@nvidia.com>
…prints#196) * test: add missing auth phase-5 regression coverage Cover remaining auth test gaps by validating HTTP/WS observability paths, SessionProvider refresh wiring, and websocket auth error propagation behavior.
Two upstream test gaps caught locally: 1. tests/aiq_agent/jobs/test_runner.py — auth_token is now the last positional arg in submit_job's job_args list (NVIDIA-AI-Blueprints#199 added it after data_sources). Update test to assert job_args[-2] == data_sources instead of [-1]. 2. frontends/aiq_api/tests/test_job_access.py::TestAuthorizeJobAccess — authorize_job_access only enforces ownership when REQUIRE_AUTH=true, but the test class never set that env var. Add an autouse monkeypatch.setenv fixture to the class so the cross-user-denied and missing-access-row tests exercise the auth-enabled branch they expect. Both are upstream issues caught by our long-running smoke; not production-code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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.
Second step of Wave 3 — auth refactor + trust boundary + trace classification + provider lifecycle hooks. Six cherry-picks from upstream/develop applied as a single coherent stack. Plus one small follow-up test fix.
The original Wave 3b scope (NVIDIA-AI-Blueprints#194/NVIDIA-AI-Blueprints#195/NVIDIA-AI-Blueprints#196) was extended to pull NVIDIA-AI-Blueprints#199/NVIDIA-AI-Blueprints#203/NVIDIA-AI-Blueprints#206 forward because NVIDIA-AI-Blueprints#194 modifies functions and imports introduced by those three commits — applying NVIDIA-AI-Blueprints#194 alone would have required substantial hand-porting and deviated from upstream verbatim. Pulling them forward keeps every commit a clean upstream cherry-pick.
bfbb46704e50f986f075bef6d25b5dda80df5c485d0d532412caa876f2685c050c58f840b073772fa0e62035504What this PR does
Principal,validate_token_with_validators,resolve_request_user,detect_internal_caller,build_request_trace_tags,user_context,is_external_request,is_headless_request,extract_auth_token. Newaiq_api.jobs.accessmodule withcreate_job_access,authorize_job_access, ownership policy.get_current_trace_tags. Newaiq_api.auth.utilsmodule (~+347 lines). New env vars:AIQ_TRACE_USER_IDENTITY_MODE,AIQ_TRACE_USER_IDENTITY_HMAC_SECRET,AIQ_TRACE_CLIENT_ID_MODE,AIQ_TRACE_CLIENT_ID_HMAC_SECRET,AIQ_TRACE_CLIENT_IP_HEADERS(all optional, off by default).JWTValidator.validate()returnstuple[user, error_code];AuthErrorcarrieserror_code; newTokenExpiredErrorandTokenInvalidErrorsubclasses. UI receives{detail, error}JSON on 401 and routes via Datadog RUM (split into action vs error severity).setIntervalfromuseAuth;SessionProvider.refetchIntervalbecomes the single refresh loop. DefaultTOKEN_REFRESH_BUFFER_MINUTES5 → 15. New regression test assertsuseAuthnever creates its ownsetInterval.AuthProviderConfiggainsonSignIn,onSession,tokenRefreshBufferSeconds,requiredEnvVars. Doc-onlyauth-example.tstemplate demonstrates all 4 hooks.test_auth_errors.py(Python) plusauthenticated-fetch.spec.ts,websocket-client.spec.ts,providers.spec.tsx(TypeScript). test: close remaining auth bug fix test coverage gaps NVIDIA-AI-Blueprints/aiq#196 also REMOVES one Starfleet-branded JSDoc example fromtypes.ts(RH-favourable).Why pull NVIDIA-AI-Blueprints#199/NVIDIA-AI-Blueprints#203/NVIDIA-AI-Blueprints#206 forward into 3b?
Wave 3b's NVIDIA-AI-Blueprints#194 directly references functions and imports introduced by those three commits:
frontends/aiq_api/src/aiq_api/auth/middleware.py— fix: auth refactor — eliminate refresh race, increase buffer, add error semantics NVIDIA-AI-Blueprints/aiq#194 modifiesvalidate_token_with_validatorsandresolve_request_user. Both added by fix auth trust boundary and enforce async job ownership NVIDIA-AI-Blueprints/aiq#199.frontends/aiq_api/src/aiq_api/auth/__init__.py— fix: auth refactor — eliminate refresh race, increase buffer, add error semantics NVIDIA-AI-Blueprints/aiq#194 expectsget_current_trace_tagsto be exported. Added by Propagate AIQ request tags to NAT spans NVIDIA-AI-Blueprints/aiq#206.frontends/aiq_api/src/aiq_api/websocket_reconnect.py— fix: auth refactor — eliminate refresh race, increase buffer, add error semantics NVIDIA-AI-Blueprints/aiq#194 references_authenticated_user. Added by fix auth trust boundary and enforce async job ownership NVIDIA-AI-Blueprints/aiq#199.Per
feedback_stay_in_sync_with_upstream.md("prefer NVIDIA verbatim"), pulling forward keeps every commit upstream-verbatim instead of hand-porting. Wave 3c reduces to "residual" — any docs not yet pulled, plus future cleanup of the legacydecode_jwt_payload.Skipped / deviated from upstream
frontends/aiq_api/src/aiq_api/websocket_reconnect.py__init__adds_user_interaction_response: asyncio.Future[TextContent] | None = None_user_interaction(UserInteraction wrapper); base class initializes it. Added one comment explaining the deviationdeploy/compose/init-db.sqljob_info,job_access,job_events,summaries, plus checkpoint tables)deploy/helm/helm-charts-k8s/aiq/files/init-db.sql(NEW)deploy/helm/deployment-k8s/values.yamlstill has the inline configMap (separate chart, separate concern, not in scope here)frontends/aiq_api/src/aiq_api/jobs/submit.pynat.front_ends.fastapi.async_jobs.job_storeimport pathfrontends/aiq_api/src/aiq_api/routes/jobs.pysubmit_authorized_jobrefactor + owner enforcementjob_argsbuild with a single helper calltests/aiq_agent/jobs/test_runner.py::test_submit_agent_job_passes_data_sourcesjob_args[-2]auth_tokenis now the last positional arg insubmit_job'sjob_args(NVIDIA-AI-Blueprints#199 added it afterdata_sources). Test mechanic, not behaviorfrontends/aiq_api/tests/test_job_access.py::TestAuthorizeJobAccessmonkeypatch.setenv("REQUIRE_AUTH", "true")fixtureauthorize_job_accessreturns the job without ownership check when REQUIRE_AUTH is unset, causingDID NOT RAISE HTTPException. Fixture makes the assumption explicitfrontends/aiq_api/tests/test_auth.py::TestJWTValidatorGetSigningKey::test_matches_key_by_kid(already in 3a)All other upstream lines applied verbatim per
feedback_stay_in_sync_with_upstream.md.Branding / vLLM exception scan
deploy/.env.example(Add request trace classification and pseudonymous ids NVIDIA-AI-Blueprints/aiq#203) — vLLM-exception territory. Diff added 7 lines ofAIQ_TRACE_*env vars at end of file; no overlap withVLLM_*,MAAS_*,KIMI_*, orNEMOTRON_*variables. Verbatim safe.docs/source/customization/configuration-reference.md(Add request trace classification and pseudonymous ids NVIDIA-AI-Blueprints/aiq#203) — fork-framing-exception territory. Adds 10 lines documenting the new env vars. Read line-by-line; no Red Hat–specific framing affected. Verbatim.docs/source/deployment/{observability,production}.md(Add request trace classification and pseudonymous ids NVIDIA-AI-Blueprints/aiq#203) — fork-framing-exception territory. New observability section + production-deployment additions. NVIDIA-neutral content. Verbatim.frontends/aiq_api/README.md(Add request trace classification and pseudonymous ids NVIDIA-AI-Blueprints/aiq#203 +58 lines) — NVIDIA-neutral observability content. Verbatim.Verification
Static
uv lock --check— 366 packages, lockfile self-consistentruff check .— cleanpytest tests/ frontends/aiq_api/tests/— 1014 passed, 19 skipped, 5.41s (up 39 from Wave 3a baseline of 975)cd frontends/ui && npm test— 1167 passed, 1 skipped (UI tests including newproviders.spec.tsx,websocket-client.spec.ts,authenticated-fetch.spec.tsfrom test: close remaining auth bug fix test coverage gaps NVIDIA-AI-Blueprints/aiq#196)cd frontends/ui && npm run type-check— cleanBroader smoke (per
feedback_upstream_sync_planning.md)Sync
/chat/streamagainst NIM (Nemotron3-Nano-30B):Async
/v1/jobs/async/submit + /streamagainst NIM:WebSocket
/websocketagainst NIM:Auth allowlist (
REQUIRE_AUTH=false):Auth gate (
REQUIRE_AUTH=true, no validators registered):✅ Refuses to start (correct behavior — prevents footgun deploys).
Tuple-return + ownership smokes
Tuple-return JSON body shape (
{detail, error}) on 401 is covered by 5+ direct unit tests intest_auth.py::TestMiddlewareErrorCodesandtest_auth.py::TestValidateErrorCodes. End-to-end requires registering a real OIDC validator; deferred to deployment-level testing.Ownership enforcement is covered by 7 tests in
test_job_access.py::TestAuthorizeJobAccess(withREQUIRE_AUTH=trueautouse fixture). End-to-end requires multi-user JWT setup; deferred to deployment-level testing.What's NOT in this PR
🤖 Generated with Claude Code