feat(authenticator): restore the __override view-as login (#1941) - #1944
Merged
cyberantonz merged 3 commits intoJul 27, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe authenticator now supports a disabled-by-default ChangesView-As Override
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant AuthLogin
participant AuthCallback
participant IdentityService
participant SessionStore
Browser->>AuthLogin: Request login with __override=email
AuthLogin->>SessionStore: Store override_email in login state
Browser->>AuthCallback: Complete authenticated OIDC callback
AuthCallback->>IdentityService: Resolve caller and override target
AuthCallback->>SessionStore: Store target session with impersonator metadata
Browser->>AuthCallback: Request /auth/me
AuthCallback-->>Browser: Return target identity and impersonator_email
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 |
cyberantonz
force-pushed
the
feat/override-login-1941
branch
from
July 27, 2026 10:10
0b507ce to
f5ec597
Compare
ktursunov
approved these changes
Jul 27, 2026
aleksdotbar
approved these changes
Jul 27, 2026
cyberantonz
added a commit
to constructorfabric/insight-front
that referenced
this pull request
Jul 27, 2026
Companion to the authenticator change (constructorfabric/insight#1944) restoring the operator 'view the dashboard as another user' facility: - The historical URL keeps working: '?__override=<email>' on any page is consumed at boot and bounced into /auth/login?__override=...&return_to= (the parameter itself never enters the router). Works while already logged in — the authenticator revokes the presented session at the callback and the IdP hop is silent SSO. - /auth/me's 'impersonator_email' (present only on view-as sessions) is parsed into the session store. - A 'Viewing as X' banner renders above the content on view-as sessions, naming the real principal, with an exit button that just re-logs-in as yourself. The override is honored server-side only where the authenticator's 'override_enabled' flag is on (dev/demo stands); everywhere else the parameter is inert and this UI never shows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…rfabric#1941) Restore the operator 'view the dashboard as another user' facility that the gateway authz hardening (constructorfabric#1769) removed as collateral, at the only seam compatible with gateway-authored identity: /auth/login?__override= <email> is stored with the transient login state and applied at /auth/callback -- after full IdP authentication and person resolution -- by resolving the target through the same Identity lookup and minting the session + linked JWT for the target person. Gated by a single override_enabled config flag (authenticator. overrideEnabled Helm value), default false at every layer; dev/demo environments only. With the flag off the parameter is inert and logged. The session record keeps the real principal (impersonator_person_id/ email) and the real idp_sub/sid/refresh token, so audit attribution, back-channel logout, and the background refresher keep targeting the real IdP grant; the session is additionally indexed under the impersonator so revoke-by-person and self 'log out everywhere' reach it. /auth/me exposes impersonator_email for the SPA 'viewing as' banner. Unknown targets are denied 403 and audited to the durable sink; the override value is sanitized before logging. Covered by e2e_override (swap + JWT sub, unknown-target 403, flag-off inertia, impersonator revoke-all) against a second flag-off authenticator instance in run-e2e.sh; PRD 5.16 + DESIGN DD-AUTH-09 document the contract and the accepted email-only-lookup tenant edge. Closes constructorfabric#1941 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Two e2e additions for the token-caching concern (review follow-up): - override A -> logout -> override B: the old cookie is refused by both /auth/me and /internal/authz after logout (nothing for the gateway exchange cache to re-serve or re-cache -- the nginx cache is keyed by the cookie value, and each login mints a fresh CSPRNG cookie, session id, and linked JWT), and the second session carries the NEW target's sub, matching /auth/me. - override A -> override B WITHOUT logout (browser-style, cookie presented on the callback): the session-fixation guard revokes the presented session before minting the new one -- the old credential dies and the new session is the new target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz
force-pushed
the
feat/override-login-1941
branch
from
July 27, 2026 10:43
457393b to
830101f
Compare
cyberantonz
enabled auto-merge (squash)
July 27, 2026 11:12
ktursunov
approved these changes
Jul 27, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/tests/run-e2e.sh (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCross-test race: revoke-all in
e2e_override.rsvs. unpinned test parallelism inrun-e2e.sh. The four tests ine2e_override.rsshare one real-principal identity (user), and one of them callsDELETE /auth/sessions(revoke-all) for that identity; cargo's default parallel test execution means this can kill a sibling test's in-flight session mid-run.
src/backend/services/authenticator/tests/run-e2e.sh#L146-150: add--test-threads=1to thecargo test -p authenticator --test e2e_overrideinvocation so the suite runs serially.src/backend/services/authenticator/tests/e2e_override.rs#L149-174: no code change needed once serialized; alternatively, use a dedicated per-test principal email to remove the shared-identity coupling entirely.🤖 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/run-e2e.sh` at line 1, The e2e_override tests share a principal while revoke-all can invalidate parallel tests; update the cargo test invocation in run-e2e.sh to pass --test-threads=1 for the e2e_override suite, leaving e2e_override.rs unchanged.
🤖 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 `@docs/components/backend/authenticator/DESIGN.md`:
- Line 786: Update the Redis HASH field list in the LoginState documentation to
use `return_to` instead of `redirect_to`, matching the field serialized by
`LoginState::to_fields()` and parsed by `LoginState::from_map()`; leave the
other fields unchanged.
In `@src/backend/services/authenticator/tests/e2e_override.rs`:
- Around line 149-174: Prevent the revoke-all flow from affecting concurrent
tests by using a test-specific identity instead of the shared default user in
the scenario around login_flow and DELETE /auth/sessions. Ensure the
impersonator and its view-as session still belong to that isolated identity so
the existing 401 assertion remains valid.
In `@src/backend/services/authenticator/tests/run-e2e.sh`:
- Around line 146-150: Update the e2e_override cargo test invocation in the run
script to pass --test-threads=1, ensuring tests sharing the user identity
execute serially while preserving the existing ignored and nocapture options.
---
Outside diff comments:
In `@src/backend/services/authenticator/tests/run-e2e.sh`:
- Line 1: The e2e_override tests share a principal while revoke-all can
invalidate parallel tests; update the cargo test invocation in run-e2e.sh to
pass --test-threads=1 for the e2e_override suite, leaving e2e_override.rs
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: c915b18c-5d80-4fa8-8713-7a5000a9d7f1
📒 Files selected for processing (10)
charts/insight/templates/secrets.yamlcharts/insight/values.yamldocs/components/backend/authenticator/DESIGN.mddocs/components/backend/authenticator/PRD.mdsrc/backend/services/authenticator/src/api/handlers.rssrc/backend/services/authenticator/src/config.rssrc/backend/services/authenticator/src/session.rssrc/backend/services/authenticator/tests/e2e_override.rssrc/backend/services/authenticator/tests/identity-stub.pysrc/backend/services/authenticator/tests/run-e2e.sh
| #### Key: `asm:login_state:{state}` | ||
|
|
||
| **Type**: Redis HASH. Fields: `pkce_verifier`, `nonce`, `redirect_to`. **TTL**: 5 minutes, one-shot. The live count is capped (layer-2 rate limiting). | ||
| **Type**: Redis HASH. Fields: `pkce_verifier`, `nonce`, `redirect_to`, `override_email` (view-as target, DD-AUTH-09; empty on normal logins). **TTL**: 5 minutes, one-shot. The live count is capped (layer-2 rate limiting). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Field name mismatch: redirect_to vs. actual return_to.
LoginState in session.rs names the field return_to (used in to_fields()/from_map()), not redirect_to.
📝 Proposed fix
-**Type**: Redis HASH. Fields: `pkce_verifier`, `nonce`, `redirect_to`, `override_email` (view-as target, DD-AUTH-09; empty on normal logins). **TTL**: 5 minutes, one-shot. The live count is capped (layer-2 rate limiting).
+**Type**: Redis HASH. Fields: `pkce_verifier`, `nonce`, `return_to`, `override_email` (view-as target, DD-AUTH-09; empty on normal logins). **TTL**: 5 minutes, one-shot. The live count is capped (layer-2 rate limiting).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Type**: Redis HASH. Fields: `pkce_verifier`, `nonce`, `redirect_to`, `override_email` (view-as target, DD-AUTH-09; empty on normal logins). **TTL**: 5 minutes, one-shot. The live count is capped (layer-2 rate limiting). | |
| **Type**: Redis HASH. Fields: `pkce_verifier`, `nonce`, `return_to`, `override_email` (view-as target, DD-AUTH-09; empty on normal logins). **TTL**: 5 minutes, one-shot. The live count is capped (layer-2 rate limiting). |
🤖 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/DESIGN.md` at line 786, Update the
Redis HASH field list in the LoginState documentation to use `return_to` instead
of `redirect_to`, matching the field serialized by `LoginState::to_fields()` and
parsed by `LoginState::from_map()`; leave the other fields unchanged.
Comment on lines
+149
to
+174
| // The view-as session is reachable through the REAL principal: it is | ||
| // indexed under both persons, so the impersonator's own "log out | ||
| // everywhere" must kill it. | ||
| let cb = login_flow(&http, &auth_base, &user, None).await; | ||
| assert_eq!(cb.status(), 302); | ||
| let own_token = cookie_from(&cb).expect("normal login must set the cookie"); | ||
| let csrf = csrf_token(&http, &auth_base, &own_token).await; | ||
| let all = http | ||
| .delete(format!("{auth_base}/auth/sessions")) | ||
| .header(reqwest::header::COOKIE, format!("{COOKIE}={own_token}")) | ||
| .header("X-CSRF-Token", &csrf) | ||
| .send() | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(all.status(), 200, "revoke-all as the impersonator"); | ||
| let dead = http | ||
| .get(format!("{auth_base}/auth/me")) | ||
| .header(reqwest::header::COOKIE, format!("{COOKIE}={token}")) | ||
| .send() | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!( | ||
| dead.status(), | ||
| 401, | ||
| "the impersonator's revoke-all must reach their view-as session" | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Revoke-all test races with sibling tests sharing the same identity.
This test authenticates user a third time and calls DELETE /auth/sessions, which revokes ALL of user's sessions. Tests 2 and 3 in this file (override_relogin_swaps_the_target_and_kills_the_old_session, override_switch_without_logout_revokes_the_presented_session) also authenticate as the same default user. Since Rust's test runner executes tests within a binary concurrently by default, and run-e2e.sh doesn't pin --test-threads=1 for this invocation, this revoke-all can intermittently kill a sibling test's in-flight session, producing flaky CI failures.
See consolidated comment for the fix location.
🤖 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_override.rs` around lines 149 -
174, Prevent the revoke-all flow from affecting concurrent tests by using a
test-specific identity instead of the shared default user in the scenario around
login_flow and DELETE /auth/sessions. Ensure the impersonator and its view-as
session still belong to that isolated identity so the existing 401 assertion
remains valid.
Comment on lines
+146
to
+150
| echo "==> run the __override view-as loop (#1941)" | ||
| AUTH_BASE="http://localhost:$AUTH_PORT" AUTH_BASE_DISABLED="http://localhost:$AUTH2_PORT" \ | ||
| E2E_USER=dev@company.nonpresent \ | ||
| cargo test -p authenticator --test e2e_override -- --ignored --nocapture | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Pin --test-threads=1 for e2e_override — its tests share a destructive, identity-scoped operation.
e2e_override.rs's first test calls DELETE /auth/sessions (revoke-all) for the shared user identity, while the sibling tests in the same binary also authenticate as that same user. Without --test-threads=1 (or RUST_TEST_THREADS=1), cargo's default parallel test execution can let the revoke-all race with and kill a sibling test's in-flight session, causing flaky CI failures.
🔧 Proposed fix
AUTH_BASE="http://localhost:$AUTH_PORT" AUTH_BASE_DISABLED="http://localhost:$AUTH2_PORT" \
E2E_USER=dev@company.nonpresent \
- cargo test -p authenticator --test e2e_override -- --ignored --nocapture
+ cargo test -p authenticator --test e2e_override -- --ignored --nocapture --test-threads=1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| echo "==> run the __override view-as loop (#1941)" | |
| AUTH_BASE="http://localhost:$AUTH_PORT" AUTH_BASE_DISABLED="http://localhost:$AUTH2_PORT" \ | |
| E2E_USER=dev@company.nonpresent \ | |
| cargo test -p authenticator --test e2e_override -- --ignored --nocapture | |
| echo "==> run the __override view-as loop (`#1941`)" | |
| AUTH_BASE="http://localhost:$AUTH_PORT" AUTH_BASE_DISABLED="http://localhost:$AUTH2_PORT" \ | |
| E2E_USER=dev@company.nonpresent \ | |
| cargo test -p authenticator --test e2e_override -- --ignored --nocapture --test-threads=1 |
🤖 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/run-e2e.sh` around lines 146 - 150,
Update the e2e_override cargo test invocation in the run script to pass
--test-threads=1, ensuring tests sharing the user identity execute serially
while preserving the existing ignored and nocapture options.
cyberantonz
added a commit
to cyberantonz/insight
that referenced
this pull request
Jul 27, 2026
Mirror of insight-gitops!34 (AGENTS.md sync rule for docs/deploy). The umbrella secrets.yaml added in constructorfabric#1944 is guarded by credentials.autoGenerate and never renders on gitops stands — insight-authenticator-config is composed by this script. Read authenticator.overrideEnabled from the env values (default false) and emit the override_enabled env var into the composed Secret. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
mitasovr
pushed a commit
that referenced
this pull request
Jul 28, 2026
#1950) Mirror of insight-gitops!34 (AGENTS.md sync rule for docs/deploy). The umbrella secrets.yaml added in #1944 is guarded by credentials.autoGenerate and never renders on gitops stands — insight-authenticator-config is composed by this script. Read authenticator.overrideEnabled from the env values (default false) and emit the override_enabled env var into the composed Secret. Signed-off-by: Anton Zelenov <antonz@constructor.tech> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 30, 2026
cyberantonz
added a commit
to cyberantonz/insight
that referenced
this pull request
Aug 5, 2026
Companion to the authenticator change (constructorfabric#1944) restoring the operator 'view the dashboard as another user' facility: - The historical URL keeps working: '?__override=<email>' on any page is consumed at boot and bounced into /auth/login?__override=...&return_to= (the parameter itself never enters the router). Works while already logged in — the authenticator revokes the presented session at the callback and the IdP hop is silent SSO. - /auth/me's 'impersonator_email' (present only on view-as sessions) is parsed into the session store. - A 'Viewing as X' banner renders above the content on view-as sessions, naming the real principal, with an exit button that just re-logs-in as yourself. The override is honored server-side only where the authenticator's 'override_enabled' flag is on (dev/demo stands); everywhere else the parameter is inert and this UI never shows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
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.
Summary
Restores the operator "view the dashboard as another user" facility (
?__override=<email>) that the gateway authz hardening (#1769) removed as collateral — re-implemented at the only seam compatible with the gateway-authored identity model: inside the authenticator, at session mint.GET /auth/login?__override=<email>stores the target with the transient login state — only when the newoverride_enabledconfig flag is on (defaultfalseat every layer; Helm:authenticator.overrideEnabled). Flag off → the parameter is inert and logged./auth/callback, after full IdP authentication and resolution of the real person, the target email is resolved through the same Identity lookup and the session + linked gateway JWT are minted for the targetperson_id/email.impersonator_person_id/email) and the realidp_sub/idp_sid/refresh token — audit attribution, back-channel logout, and the background refresher keep targeting the real IdP grant./auth/meexposesimpersonator_emailfor the SPA "viewing as" banner (FE follow-up in insight-front).__overridevalue is sanitized (control chars stripped, length capped) before the one log path reachable in flag-off environments.No #1769 regression: no client-supplied header/param is ever trusted as identity; every decision input is server-side (flag, login-state value, person store), and the caller still fully authenticates at the IdP.
Accepted edge (documented in DD-AUTH-09): Identity's internal lookup is email-only (no tenant memberships until #1687), so a target from another tenant resolves and is paired with the caller's tenant claim — acceptable while the flag marks whole single-tenant dev/demo environments.
Config
Testing
run-e2e.shsuite passes, including newe2e_override: identity swap (JWTsub+/auth/me), unknown-target 403, flag-off inertia (second authenticator instance at default config), impersonator revoke-all reaching the view-as session.APP__gears__authenticator__config__override_enabled: "true".cpt-insightspec-fr-auth-override) + DESIGN DD-AUTH-09;cfs validate+check-languagepass on both artifacts.Closes #1941
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
__override=<email>parameter./auth/meresponses.Security
Tests