feat(auth): cookie/BFF session model — drop client-side OIDC (NGINX_BFF #1583)#205
Open
cyberantonz wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe frontend replaces OIDC authentication with gateway-managed cookie sessions, adds ChangesCookie Authentication Cutover
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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: 3
🧹 Nitpick comments (3)
Dockerfile (1)
16-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant custom entrypoint and copy configuration directly.
With all runtime OIDC templating logic removed, the custom
docker-entrypoint.shis functionally redundant. Currently, copying it into the container overwrites the official NGINX base image's standard/docker-entrypoint.sh, which bypasses its default lifecycle hooks (like IPv6 defaults and worker tuning).You can simplify the build by deleting the script entirely, copying the NGINX configuration directly to
conf.dduring the Docker build, and letting the official image handle the entrypoint initialization natively.
Dockerfile#L16-L31: Apply the diff below to remove the custom entrypoint instructions and copy the template straight to the configuration folder.docker-entrypoint.sh#L4-L15: Delete this file entirely as it is no longer needed.♻️ Proposed refactor for Dockerfile
-COPY docker-entrypoint.sh /docker-entrypoint.sh -RUN chmod +x /docker-entrypoint.sh - # nginx config: security-headers snippet (included at server level AND inside # any location block that declares its own add_header — nginx replaces rather -# than merges across levels) + default.conf that the entrypoint copies into -# conf.d at container start. No runtime templating anymore (the SPA is -# same-origin only), but the file keeps its .template name and location. +# than merges across levels) + default.conf. RUN mkdir -p /etc/nginx/snippets COPY nginx/security-headers.conf /etc/nginx/snippets/security-headers.conf -COPY nginx/default.conf.template /etc/nginx/templates/default.conf.template +COPY nginx/default.conf.template /etc/nginx/conf.d/default.conf EXPOSE 80 - -ENTRYPOINT ["/docker-entrypoint.sh"] -CMD ["nginx", "-g", "daemon off;"](You may also want to optionally rename
nginx/default.conf.templatetonginx/default.confin your repository for clarity, though leaving the current filename works fine).🤖 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 `@Dockerfile` around lines 16 - 31, In Dockerfile, remove the custom docker-entrypoint.sh copy, chmod, and ENTRYPOINT instructions; copy nginx/default.conf.template directly into /etc/nginx/conf.d/default.conf and retain the official NGINX CMD. Delete docker-entrypoint.sh entirely, as no runtime templating remains; renaming the template file is optional and not required.src/mocks/handlers.ts (1)
386-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicated type definition.
The union type has a duplicated
{ value_type?: string; value?: string }object shape.♻️ Proposed refactor
const body = (await request.json().catch(() => null)) as | { value_type?: string; value?: string } - | { value_type?: string; value?: string } | null;🤖 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/mocks/handlers.ts` around lines 386 - 388, Update the request body type annotation in the handler around request.json() to define the `{ value_type?: string; value?: string }` object shape only once, removing the duplicated union member while preserving the nullable result.src/auth/use-viewer.ts (1)
15-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMemoize the returned object to preserve referential equality.
Currently,
resolve()creates a new object on every render, which can trigger unnecessary re-renders in downstream components that depend onuseViewer()(e.g., insideuseEffectdependencies orReact.memo). Consider using the returned snapshot fromuseSyncExternalStorealong withuseMemoto return a stable reference.♻️ Proposed refactor
Assuming
useMemois imported fromreact:-function resolve(): Viewer { - const { session } = authStore.getSnapshot(); - return { - email: session?.email ?? null, - personId: session?.personId ?? null, - }; -} - export function useViewer(): Viewer { - useSyncExternalStore( + const { session } = useSyncExternalStore( authStore.subscribe, authStore.getSnapshot, authStore.getSnapshot, ); - return resolve(); + return useMemo(() => ({ + email: session?.email ?? null, + personId: session?.personId ?? null, + }), [session?.email, session?.personId]); }🤖 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/auth/use-viewer.ts` around lines 15 - 30, Memoize the viewer object returned by useViewer to preserve referential equality across renders. Use the snapshot returned by useSyncExternalStore as the source values and wrap the constructed Viewer in useMemo, updating it only when the relevant snapshot changes; remove the separate resolve() call from the return path while preserving the existing email and personId fallbacks.
🤖 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/api/identity-client.ts`:
- Around line 61-78: Update toIdentityPerson so profiles with a missing or empty
ProfileResponse.email are rejected instead of being converted with email set to
"". Preserve the existing mapping for valid profiles and ensure recursive
subordinate mapping applies the same validation.
In `@src/auth/session.ts`:
- Around line 11-39: The loadSession function lacks unit-test coverage for
successful authentication, non-OK responses, and network failures. Add tests
targeting loadSession that mock fetch and authStore to verify parsed user data
and authenticated status on success, unauthenticated status and store update for
error responses, and the same fail-closed behavior when fetch rejects; cover
both response handling and exception paths.
In `@src/auth/use-auth.ts`:
- Around line 29-43: Update signOut to require a successful logout response by
checking res.ok after the fetch; on network or non-OK failure, do not call
authStore.setUnauthenticated or redirect as a successful logout, and surface or
retry the failure as appropriate. Preserve the existing rp_logout_url
destination only for successful responses.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 16-31: In Dockerfile, remove the custom docker-entrypoint.sh copy,
chmod, and ENTRYPOINT instructions; copy nginx/default.conf.template directly
into /etc/nginx/conf.d/default.conf and retain the official NGINX CMD. Delete
docker-entrypoint.sh entirely, as no runtime templating remains; renaming the
template file is optional and not required.
In `@src/auth/use-viewer.ts`:
- Around line 15-30: Memoize the viewer object returned by useViewer to preserve
referential equality across renders. Use the snapshot returned by
useSyncExternalStore as the source values and wrap the constructed Viewer in
useMemo, updating it only when the relevant snapshot changes; remove the
separate resolve() call from the return path while preserving the existing email
and personId fallbacks.
In `@src/mocks/handlers.ts`:
- Around line 386-388: Update the request body type annotation in the handler
around request.json() to define the `{ value_type?: string; value?: string }`
object shape only once, removing the duplicated union member while preserving
the nullable result.
🪄 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
Run ID: 48f19099-6e80-4c24-a6e9-9c522d53cae1
📒 Files selected for processing (43)
Dockerfiledocker-compose.ymldocker-entrypoint.shnginx/default.conf.templatenginx/security-headers.confsrc/api/catalog-provider.tsxsrc/api/fetch-with-auth.test.tssrc/api/fetch-with-auth.tssrc/api/identity-client.tssrc/api/use-catalog.test.tsxsrc/api/use-catalog.tssrc/api/view-configs.test.tsxsrc/auth/auth-store.tssrc/auth/config.tssrc/auth/dev-config.tssrc/auth/impersonation.tssrc/auth/index.tssrc/auth/oidc-manager.test.tssrc/auth/oidc-manager.tssrc/auth/session.tssrc/auth/start-url.tssrc/auth/types.tssrc/auth/use-auth.tssrc/auth/use-viewer.tssrc/components/app-sidebar.tsxsrc/components/auth-gate.test.tsxsrc/components/auth-gate.tsxsrc/components/dev-impersonation-hint.tsxsrc/components/impersonation-banner.tsxsrc/components/widgets/v2/counters-block.test.tsxsrc/components/widgets/v2/distribution-strip.test.tsxsrc/components/widgets/v2/members-heatmap/index.test.tsxsrc/components/widgets/v2/section-card.test.tsxsrc/components/widgets/v2/team-members-attention.test.tsxsrc/locales/en/translation.jsonsrc/main.tsxsrc/mocks/handlers.tssrc/routeTree.gen.tssrc/routes/__root.tsxsrc/routes/callback.tsxsrc/routes/index.tsxsrc/test/catalog-test-utils.tsxvite.config.ts
💤 Files with no reviewable changes (10)
- src/components/dev-impersonation-hint.tsx
- src/routes/callback.tsx
- src/auth/config.ts
- src/components/impersonation-banner.tsx
- src/auth/impersonation.ts
- src/auth/dev-config.ts
- src/auth/oidc-manager.test.ts
- src/auth/oidc-manager.ts
- src/auth/start-url.ts
- src/routeTree.gen.ts
cyberantonz
force-pushed
the
nginx-auth-step-07-downstream-verify
branch
from
July 17, 2026 15:09
6460736 to
0e529d7
Compare
…FF #1583)
The SPA no longer runs OIDC in the browser. Auth is now cookie-based behind the
nginx gateway: the browser holds only the opaque __Host-sid session cookie, the
gateway injects the ES256 gateway JWT downstream, and the SPA sees no tokens.
- Bootstrap: GET /auth/me once at boot (returns user/email/tenants/roles);
signIn = full-page redirect to /auth/login?return_to=…; signOut = POST
/auth/logout then follow rp_logout_url.
- fetchWithAuth: credentials:'include', no Authorization/X-Tenant-ID header; a
401 marks the store unauthenticated and bounces to /auth/login (no client-side
token/refresh).
- auth-store/types reshaped to a { status, session } summary; use-viewer reads
the session; the catalog tenant comes from session.tenants[0].
- Delete all oidc-client-ts usage: oidc-manager, config (__OIDC_CONFIG__),
dev-config/impersonation (__DEV_CONFIG__/VITE_DEV_USER_EMAIL), start-url, the
/callback route, and the dev-impersonation hint + banner.
- identity-client: POST /api/identity/v1/profiles (the GET /persons/{email} it
used is deprecated server-side).
- Serve/build: docker-entrypoint no longer injects oidc-config.js; CSP tightened
to 'self'; vite dev proxies /auth alongside /api to the gateway.
- Tests/mocks/i18n updated (MSW /auth/me + /auth/logout; auth.redirecting).
Pairs with the authenticator emitting `email` from /auth/me. Verified: pnpm
build (whole-project tsc + vite) green, pnpm test 285/285.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
identity-client: reject a profile missing the required email (the UI identity /
React key) instead of projecting an unusable empty-string person, and drop
email-less subordinates rather than colliding on duplicate "" keys. Add
identity-client.test.ts covering the mapping + all error paths (closes the
diff-coverage gate on the new POST /v1/profiles path).
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Add auth-cutover unit tests (session.loadSession, use-auth.signOut,
use-viewer.getViewerEmail) to close the diff-coverage gate on the new
cookie/BFF modules.
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz
force-pushed
the
nginx-auth-step-07-downstream-verify
branch
from
July 17, 2026 15:15
0e529d7 to
58640b1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/auth/use-auth.ts (1)
29-44: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDo not report logout success when the session revocation fails.
Network failures and non-OK responses still clear local state and redirect to
/. Because the valid cookie might remain,/auth/mewill immediately authenticate the user again on the next load. The shared root cause is thatsignOutincorrectly swallows errors and the test suite rigidly expects this flawed behavior.
src/auth/use-auth.ts#L29-L44: Checkres.okand surface the failure instead of completing the logout flow and callingauthStore.setUnauthenticated().src/auth/use-auth.test.ts#L56-L64: Update the test to verify that the error is propagated and the session remains authenticated.🤖 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/auth/use-auth.ts` around lines 29 - 44, Update signOut in src/auth/use-auth.ts:29-44 to check res.ok, propagate network and non-OK failures, and only clear authStore and redirect after successful revocation; retain the logout destination handling for successful responses. Update the corresponding test in src/auth/use-auth.test.ts:56-64 to expect the error and verify the session remains authenticated.
🧹 Nitpick comments (1)
Dockerfile (1)
16-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify NGINX configuration placement.
Since runtime templating is no longer required, you can simplify the Docker image by copying the NGINX configuration directly to
conf.dat build time. This allows you to completely remove the customdocker-entrypoint.shscript and rely on the official NGINX base entrypoint, reducing runtime overhead and maintenance.♻️ Proposed refactor to eliminate the custom entrypoint
- COPY docker-entrypoint.sh /docker-entrypoint.sh - RUN chmod +x /docker-entrypoint.sh - - # nginx config: security-headers snippet (included at server level AND inside - # any location block that declares its own add_header — nginx replaces rather - # than merges across levels) + default.conf that the entrypoint copies into - # conf.d at container start. No runtime templating anymore (the SPA is - # same-origin only), but the file keeps its .template name and location. + # nginx config: security-headers snippet (included at server level AND inside + # any location block that declares its own add_header — nginx replaces rather + # than merges across levels) + default.conf that is copied directly into conf.d. + # No runtime templating is needed as the SPA is same-origin only. RUN mkdir -p /etc/nginx/snippets COPY nginx/security-headers.conf /etc/nginx/snippets/security-headers.conf - COPY nginx/default.conf.template /etc/nginx/templates/default.conf.template + COPY nginx/default.conf.template /etc/nginx/conf.d/default.conf EXPOSE 80 - - ENTRYPOINT ["/docker-entrypoint.sh"]If you apply this change, you can also safely delete the
docker-entrypoint.shfile from the repository.🤖 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 `@Dockerfile` around lines 16 - 30, Remove the custom docker-entrypoint.sh setup and ENTRYPOINT declaration, copy nginx/default.conf.template directly to /etc/nginx/conf.d/default.conf during the image build, and retain the security-headers.conf snippet placement. Delete docker-entrypoint.sh from the repository so the official NGINX base entrypoint is used.
🤖 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.
Duplicate comments:
In `@src/auth/use-auth.ts`:
- Around line 29-44: Update signOut in src/auth/use-auth.ts:29-44 to check
res.ok, propagate network and non-OK failures, and only clear authStore and
redirect after successful revocation; retain the logout destination handling for
successful responses. Update the corresponding test in
src/auth/use-auth.test.ts:56-64 to expect the error and verify the session
remains authenticated.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 16-30: Remove the custom docker-entrypoint.sh setup and ENTRYPOINT
declaration, copy nginx/default.conf.template directly to
/etc/nginx/conf.d/default.conf during the image build, and retain the
security-headers.conf snippet placement. Delete docker-entrypoint.sh from the
repository so the official NGINX base entrypoint is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f03179ee-c3ea-463d-981b-2b5313e79f2b
📒 Files selected for processing (47)
Dockerfiledocker-compose.ymldocker-entrypoint.shnginx/default.conf.templatenginx/security-headers.confsrc/api/catalog-provider.tsxsrc/api/fetch-with-auth.test.tssrc/api/fetch-with-auth.tssrc/api/identity-client.test.tssrc/api/identity-client.tssrc/api/use-catalog.test.tsxsrc/api/use-catalog.tssrc/api/view-configs.test.tsxsrc/auth/auth-store.tssrc/auth/config.tssrc/auth/dev-config.tssrc/auth/impersonation.tssrc/auth/index.tssrc/auth/oidc-manager.test.tssrc/auth/oidc-manager.tssrc/auth/session.test.tssrc/auth/session.tssrc/auth/start-url.tssrc/auth/types.tssrc/auth/use-auth.test.tssrc/auth/use-auth.tssrc/auth/use-viewer.test.tssrc/auth/use-viewer.tssrc/components/app-sidebar.tsxsrc/components/auth-gate.test.tsxsrc/components/auth-gate.tsxsrc/components/dev-impersonation-hint.tsxsrc/components/impersonation-banner.tsxsrc/components/widgets/v2/counters-block.test.tsxsrc/components/widgets/v2/distribution-strip.test.tsxsrc/components/widgets/v2/members-heatmap/index.test.tsxsrc/components/widgets/v2/section-card.test.tsxsrc/components/widgets/v2/team-members-attention.test.tsxsrc/locales/en/translation.jsonsrc/main.tsxsrc/mocks/handlers.tssrc/routeTree.gen.tssrc/routes/__root.tsxsrc/routes/callback.tsxsrc/routes/index.tsxsrc/test/catalog-test-utils.tsxvite.config.ts
💤 Files with no reviewable changes (10)
- src/auth/dev-config.ts
- src/auth/oidc-manager.test.ts
- src/auth/start-url.ts
- src/components/dev-impersonation-hint.tsx
- src/routes/callback.tsx
- src/auth/config.ts
- src/components/impersonation-banner.tsx
- src/auth/impersonation.ts
- src/auth/oidc-manager.ts
- src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (30)
- nginx/security-headers.conf
- src/test/catalog-test-utils.tsx
- src/routes/__root.tsx
- docker-compose.yml
- src/components/widgets/v2/counters-block.test.tsx
- nginx/default.conf.template
- src/api/use-catalog.ts
- src/api/fetch-with-auth.ts
- src/components/widgets/v2/team-members-attention.test.tsx
- src/api/catalog-provider.tsx
- src/components/widgets/v2/section-card.test.tsx
- src/routes/index.tsx
- src/components/widgets/v2/members-heatmap/index.test.tsx
- src/auth/session.ts
- src/api/view-configs.test.tsx
- docker-entrypoint.sh
- src/mocks/handlers.ts
- src/components/widgets/v2/distribution-strip.test.tsx
- src/auth/auth-store.ts
- src/api/use-catalog.test.tsx
- src/components/app-sidebar.tsx
- src/auth/index.ts
- src/components/auth-gate.test.tsx
- src/locales/en/translation.json
- src/auth/use-viewer.ts
- src/api/identity-client.ts
- src/api/fetch-with-auth.test.ts
- src/auth/types.ts
- src/main.tsx
- src/components/auth-gate.tsx
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
1 participant
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.
What
Refactors the SPA from client-side OIDC (
oidc-client-ts) to the server-side cookie/BFF session model introduced by the nginx+auth edge (EPIC constructorfabric/insight#1583).The browser now authenticates entirely through the gateway edge:
/auth/login→ gateway → authenticator → IdP →__Host-sidsession cookie/api/*and/auth/mesame-origin withcredentials: 'include'401triggerssignIn()(redirect to/auth/login)There is no client-side OIDC, no token handling in the browser, and no runtime config injection anymore.
Changes
auth-store,session,use-auth,use-viewer,types; deletedoidc-manager,config,dev-config,impersonation,start-urland the client-OIDC callback route.fetch-with-auth(credentials: 'include',401 → signIn);identity-clientnow usesPOST /v1/profiles(the deprecatedGET /v1/persons/{email}is gone)./auth/me(now includesemail).docker-entrypoint.sh/nginx/default.conf.template/vite.config.tsdropoidc-config.js, tighten CSP to'self', and proxy/auth+/apito the gateway.Testing
pnpm build+ full unit suite green.__Host-sidcookie and/auth/mereturns{user, email, tenants, roles, ...};/api/*served same-origin.Sequencing
Pairs with the backend step-07 PR constructorfabric/insight#1777 and is the SPA cutover (step 08, constructorfabric/insight#1591) for EPIC constructorfabric/insight#1583. Must land together with the backend — the app 401s until both sides are deployed.
🤖 Generated with Claude Code
Summary by CodeRabbit
connect-src/frame-srcsame-origin in the default configuration.