security(auth): gate SSO settings page + API by enterprise edition - #934
Merged
Conversation
Resolves the gap where the SSO management UI rendered fully on community
installs. The existing inline NEOBOARD_EDITION check on /api/sso-providers
was kept (already returned 403 forbidden) — this PR replaces it with the
canonical requireFeature("sso") guard (returns 402 ENTERPRISE_REQUIRED)
and adds defense-in-depth + a client-side gate so the UI itself never
renders on community.
Changes:
- /api/sso-providers (admin CRUD): replace inline edition check with
requireFeature("sso"); now returns 402 ENTERPRISE_REQUIRED instead of 403
- /api/auth/sso-providers (public login route): short-circuit to empty
response on community before any DB read (defense-in-depth — even stale
rows or env-provider misconfig can't leak)
- New useFeatures()/useFeature() hook: TanStack Query, 5-min staleTime,
reads /api/features
- New <FeatureGate feature="..."> component for declarative client gating
- New <EnterpriseRequiredEmptyState feature="..."> reusable empty state
with auto-generated copy per feature + upgrade CTA
- Settings layout: filter Authentication tab by sso feature; hidden on
community (avoids dead-end and UI flicker during initial load)
- Settings/authentication page: wrap content in FeatureGate; community
users see the EnterpriseRequiredEmptyState directly
Tests:
- 49 unit tests pass (6 new useFeatures + 6 FeatureGate + 1 new community-
edition page test + updated route tests + all pre-existing tests)
- New E2E spec app/e2e/sso-gating.spec.ts covers community-mode UI +
API gating
- Enterprise-mode E2E coverage filed as #933 (requires second Playwright
worker — substantial global-setup overhaul, out of scope)
Drill brief: claude_code_docs/plans/issue-906.md
Closes #906
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alfredo1996
added
security
WalkthroughThis PR gates SSO provider management to enterprise via a feature-flag system: adds client hooks ( ChangesSSO Community Edition Gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labelsenhancement 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
This was referenced Jun 4, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/app/api/sso-providers/__tests__/route.test.ts (1)
169-181:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin
NEOBOARD_EDITIONin POST/PATCHbeforeEachto avoid env-dependent test behavior.Line 169 and Line 444 import the route without stubbing edition, so these suites can silently flip behavior if ambient env is community (returning 402 earlier than expected).
Suggested patch
beforeEach(async () => { vi.resetModules(); vi.clearAllMocks(); @@ vi.doMock("`@/lib/crypto/crypto`", () => ({ encrypt: mockEncrypt })); vi.doMock("next/server", () => nextResponseMockFactory()); vi.mock("`@/lib/auth/errors`", () => ({ UnauthorizedError, ForbiddenError })); + vi.stubEnv("NEOBOARD_EDITION", "enterprise"); const mod = await import("../route"); POST = mod.POST; }); @@ beforeEach(async () => { vi.resetModules(); vi.clearAllMocks(); @@ vi.doMock("`@/lib/crypto/crypto`", () => ({ encrypt: mockEncrypt })); vi.doMock("next/server", () => nextResponseMockFactory()); vi.doMock("`@/lib/auth/sso/provider-cache`", () => ({ invalidateProviderCache: mockInvalidateCache, })); + vi.stubEnv("NEOBOARD_EDITION", "enterprise"); const mod = await import("../route"); PATCH = mod.PATCH; });Also applies to: 444-458
🤖 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 `@app/src/app/api/sso-providers/__tests__/route.test.ts` around lines 169 - 181, The tests import the route in beforeEach (assigning POST from the module) without stubbing NEOBOARD_EDITION, causing environment-dependent behavior; fix by setting process.env.NEOBOARD_EDITION to a deterministic value (e.g., "enterprise") at the start of the beforeEach that imports "../route" (the block that sets POST) and restore or delete the env var in afterEach; ensure the same change is applied to the analogous beforeEach that imports PATCH (around lines 444–458) so both POST and PATCH suites run deterministically.
🧹 Nitpick comments (2)
app/e2e/sso-gating.spec.ts (1)
45-52: ⚡ Quick winUse
page.requesthere to validate the logged-in flow you set up inbeforeEach.Line 48 currently uses the global
requestcontext, so this assertion checks anonymous behavior. Switching topage.request.get(...)makes the API check consistent with the authenticated user journey in this describe block.🤖 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 `@app/e2e/sso-gating.spec.ts` around lines 45 - 52, The test named "/api/sso-providers returns 402 ENTERPRISE_REQUIRED" is using the global request context (request.get) which validates anonymous behavior; switch to using the authenticated request context created for the page by calling page.request.get(...) so the API call runs as the logged-in user established in beforeEach. Update the call inside that test from request.get("/api/sso-providers") to page.request.get("/api/sso-providers") and keep the subsequent assertions (res.status and body.error.code) unchanged.app/src/app/api/sso-providers/__tests__/route.test.ts (1)
90-109: ⚡ Quick winAdd 402 contract tests for POST/DELETE/PATCH as well.
Only GET currently asserts the new
ENTERPRISE_REQUIREDcontract. Since Line 81, Line 169, and Line 197 were changed too, adding parallel assertions will prevent regressions in the full CRUD surface.🤖 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 `@app/src/app/api/sso-providers/__tests__/route.test.ts` around lines 90 - 109, Add equivalent contract tests for POST, DELETE and PATCH in app/src/app/api/sso-providers/__tests__/route.test.ts mirroring the existing GET test: for each verb re-stub NEOBOARD_EDITION to "" and re-import "../route" (same vi.resetModules/vi.doMock pattern using mockRequireAdmin, mockDb, mockEncrypt, mockInvalidateCache and nextResponseMockFactory), call mod.POST()/mod.DELETE()/mod.PATCH(), assert res.status === 402, parse res.json() and assert body.error.code === "ENTERPRISE_REQUIRED" and body.error.message matches /sso|enterprise/i so the full CRUD surface enforces the ENTERPRISE_REQUIRED contract.
🤖 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.
Outside diff comments:
In `@app/src/app/api/sso-providers/__tests__/route.test.ts`:
- Around line 169-181: The tests import the route in beforeEach (assigning POST
from the module) without stubbing NEOBOARD_EDITION, causing
environment-dependent behavior; fix by setting process.env.NEOBOARD_EDITION to a
deterministic value (e.g., "enterprise") at the start of the beforeEach that
imports "../route" (the block that sets POST) and restore or delete the env var
in afterEach; ensure the same change is applied to the analogous beforeEach that
imports PATCH (around lines 444–458) so both POST and PATCH suites run
deterministically.
---
Nitpick comments:
In `@app/e2e/sso-gating.spec.ts`:
- Around line 45-52: The test named "/api/sso-providers returns 402
ENTERPRISE_REQUIRED" is using the global request context (request.get) which
validates anonymous behavior; switch to using the authenticated request context
created for the page by calling page.request.get(...) so the API call runs as
the logged-in user established in beforeEach. Update the call inside that test
from request.get("/api/sso-providers") to page.request.get("/api/sso-providers")
and keep the subsequent assertions (res.status and body.error.code) unchanged.
In `@app/src/app/api/sso-providers/__tests__/route.test.ts`:
- Around line 90-109: Add equivalent contract tests for POST, DELETE and PATCH
in app/src/app/api/sso-providers/__tests__/route.test.ts mirroring the existing
GET test: for each verb re-stub NEOBOARD_EDITION to "" and re-import "../route"
(same vi.resetModules/vi.doMock pattern using mockRequireAdmin, mockDb,
mockEncrypt, mockInvalidateCache and nextResponseMockFactory), call
mod.POST()/mod.DELETE()/mod.PATCH(), assert res.status === 402, parse res.json()
and assert body.error.code === "ENTERPRISE_REQUIRED" and body.error.message
matches /sso|enterprise/i so the full CRUD surface enforces the
ENTERPRISE_REQUIRED contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1039ba3-13e6-43a0-af42-0af71cbcc087
📒 Files selected for processing (13)
app/e2e/sso-gating.spec.tsapp/src/app/(dashboard)/settings/authentication/__tests__/page.test.tsxapp/src/app/(dashboard)/settings/authentication/page.tsxapp/src/app/(dashboard)/settings/layout.tsxapp/src/app/api/auth/sso-providers/__tests__/route.test.tsapp/src/app/api/auth/sso-providers/route.tsapp/src/app/api/sso-providers/__tests__/route.test.tsapp/src/app/api/sso-providers/route.tsapp/src/components/__tests__/feature-gate.test.tsxapp/src/components/enterprise-required-empty-state.tsxapp/src/components/feature-gate.tsxapp/src/hooks/__tests__/use-features.test.tsapp/src/hooks/use-features.ts
Three fixes from the CodeRabbit review: 1. POST + PATCH beforeEach in route.test.ts now pin NEOBOARD_EDITION=enterprise (deterministic across ambient envs; previously could flip to 402 if community env leaked in) 2. Added 402 ENTERPRISE_REQUIRED contract tests for POST, DELETE, PATCH — previously only GET covered the new contract; now full CRUD surface is locked 3. sso-gating.spec.ts: /api/sso-providers test uses page.request.get(...) instead of the global request context. Fixes the CI ECONNRESET seen on E2E shard 4/5 (global request raced with server startup) and matches the authenticated journey set up in the describe block's beforeEach All 28 route tests pass. Local E2E deferred per user direction (Docker teardown cost outweighs benefit for this small fix-up; CI will verify). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
Addressed CodeRabbit feedback + E2E failure (commit 30cecbf)3 CR findings — all valid, all fixed:
E2E shard 4/5 ECONNRESET — the fix above ( Tests: 28/28 route tests pass locally. Full E2E deferred to CI (per session decision — local Docker teardown cost outweighs benefit for this fix-up). Watching the next CI run for shard 4/5 confirmation. |
…eadonly Addresses two SonarCloud findings on #934 (rule typescript:S6759, MINOR): - app/src/components/feature-gate.tsx:34 - app/src/components/enterprise-required-empty-state.tsx:78 Per the codebase pattern (e.g. save-template-dialog.tsx, dashboard-picker-dialog.tsx), mark each prop interface field with the `readonly` modifier. 6 feature-gate tests pass. Local E2E deferred to CI (no functional change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
SonarCloud findings addressed (commit ff8e0f9)
Pattern matches existing codebase usage ( CR open review threads (if any) — the initial 3 findings were already addressed in commit 30cecbf. No new CR review posted yet for that commit; will re-check after CI completes on ff8e0f9. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/app/api/sso-providers/__tests__/route.test.ts (1)
90-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProvide request argument for consistency and type safety.
Line 104 calls
mod.GET()without a request argument, while:
- The type signature (line 71) expects
(req: Request)- Other GET tests provide
makeRequest(null)(line 113)- The POST/DELETE/PATCH community edition tests all provide a request (lines 199, 434, 519)
While this works because
requireFeature("sso")throws before request processing, it's inconsistent with the test suite pattern.✨ Proposed fix
const mod = await import("../route"); - const res = await mod.GET(); + const res = await mod.GET(makeRequest(null)); expect(res.status).toBe(402);🤖 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 `@app/src/app/api/sso-providers/__tests__/route.test.ts` around lines 90 - 109, The test calls mod.GET() without a Request which is inconsistent with the GET signature and other tests; update the test to call mod.GET(makeRequest(null)) so the request argument is provided for type safety and consistency (modify the spec in app/src/app/api/sso-providers/__tests__/route.test.ts where mod.GET() is invoked), keeping the rest of the mocking (mockRequireAdmin, mockDb, mockEncrypt, nextResponseMockFactory, mockInvalidateCache) unchanged.
🧹 Nitpick comments (2)
app/src/app/api/sso-providers/__tests__/route.test.ts (2)
421-440: 💤 Low valueConsider validating error message for consistency.
The DELETE and PATCH community edition tests validate
statusanderror.codebut noterror.message, while GET (line 108) and POST (line 203) include.toMatch(/sso|enterprise/i). Adding message validation would improve test consistency, though the error code check is sufficient for correctness.Also applies to: 506-523
🤖 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 `@app/src/app/api/sso-providers/__tests__/route.test.ts` around lines 421 - 440, The community-edition DELETE and PATCH tests currently assert status and body.error.code but not the error message; update those tests (the one invoking mod.DELETE with makeRequest and the corresponding mod.PATCH test) to also assert the error message for consistency by adding an assertion like expect(body.error.message).toMatch(/sso|enterprise/i) after the existing expect(body.error.code) checks so they match the GET and POST tests that validate message content.
85-85: 💤 Low valueConsider adding explanatory comments for consistency.
Lines 85 and 416 pin
NEOBOARD_EDITIONto"enterprise"without comments, while POST (lines 179-181) and PATCH (lines 500-501) include comments explaining the deterministic test intent. Adding similar comments would improve consistency.Also applies to: 416-416
🤖 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 `@app/src/app/api/sso-providers/__tests__/route.test.ts` at line 85, Two instances of vi.stubEnv("NEOBOARD_EDITION", "enterprise") in the SSO providers tests lack explanatory comments; update the test file (route.test.ts) to add brief comments next to each vi.stubEnv("NEOBOARD_EDITION", "enterprise") call (the one near the test file start and the later one around the other test block) stating that the environment is pinned to "enterprise" to make the test deterministic (similar to the existing comments in the POST and PATCH test blocks), so future readers understand why the env override is required.
🤖 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.
Outside diff comments:
In `@app/src/app/api/sso-providers/__tests__/route.test.ts`:
- Around line 90-109: The test calls mod.GET() without a Request which is
inconsistent with the GET signature and other tests; update the test to call
mod.GET(makeRequest(null)) so the request argument is provided for type safety
and consistency (modify the spec in
app/src/app/api/sso-providers/__tests__/route.test.ts where mod.GET() is
invoked), keeping the rest of the mocking (mockRequireAdmin, mockDb,
mockEncrypt, nextResponseMockFactory, mockInvalidateCache) unchanged.
---
Nitpick comments:
In `@app/src/app/api/sso-providers/__tests__/route.test.ts`:
- Around line 421-440: The community-edition DELETE and PATCH tests currently
assert status and body.error.code but not the error message; update those tests
(the one invoking mod.DELETE with makeRequest and the corresponding mod.PATCH
test) to also assert the error message for consistency by adding an assertion
like expect(body.error.message).toMatch(/sso|enterprise/i) after the existing
expect(body.error.code) checks so they match the GET and POST tests that
validate message content.
- Line 85: Two instances of vi.stubEnv("NEOBOARD_EDITION", "enterprise") in the
SSO providers tests lack explanatory comments; update the test file
(route.test.ts) to add brief comments next to each
vi.stubEnv("NEOBOARD_EDITION", "enterprise") call (the one near the test file
start and the later one around the other test block) stating that the
environment is pinned to "enterprise" to make the test deterministic (similar to
the existing comments in the POST and PATCH test blocks), so future readers
understand why the env override is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae665c28-6129-479e-b32f-3c3bcd486eec
📒 Files selected for processing (4)
app/e2e/sso-gating.spec.tsapp/src/app/api/sso-providers/__tests__/route.test.tsapp/src/components/enterprise-required-empty-state.tsxapp/src/components/feature-gate.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/components/enterprise-required-empty-state.tsx
- app/src/components/feature-gate.tsx
- app/e2e/sso-gating.spec.ts
|
This was referenced Jun 4, 2026
5 tasks
alfredo1996
added a commit
that referenced
this pull request
Jun 5, 2026
…, #898, #899) (#939) * chore(.claude): polish skills, agents, CLAUDE.md for release/1.1 Findings from pre-polish review (umbrella #895): - code: add E2E to after-coding; release/1.1 branch awareness - next: auto-detect release/X.Y as base branch (instead of hard-coded dev) - github-workflow: fix frontmatter name mismatch (was 'github'); expand label list to match repo - issue: expand label list to match real GH labels (a11y, design, devex, etc.) - code-reviewer: add E2E test step (was unit-only) - test-runner: sharpen Docker conditional; clarify destroy-before-E2E rule - ux-crawler + user-sim-creator: replace ghost user 'bob@example.com' with seeded 'creator@neoboard.local' (pending #921) - CLAUDE.md: /github -> /github-workflow skill ref; clarify code-reviewer test scope - NEW: skills/deploy — production-readiness audit skill (capture-don't-fix, 5 sections, destructive-step approval gate) Pre-polish review issues filed: #921, #922, #923, #924, #925. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security(auth): gate SSO settings page + API by enterprise edition Resolves the gap where the SSO management UI rendered fully on community installs. The existing inline NEOBOARD_EDITION check on /api/sso-providers was kept (already returned 403 forbidden) — this PR replaces it with the canonical requireFeature("sso") guard (returns 402 ENTERPRISE_REQUIRED) and adds defense-in-depth + a client-side gate so the UI itself never renders on community. Changes: - /api/sso-providers (admin CRUD): replace inline edition check with requireFeature("sso"); now returns 402 ENTERPRISE_REQUIRED instead of 403 - /api/auth/sso-providers (public login route): short-circuit to empty response on community before any DB read (defense-in-depth — even stale rows or env-provider misconfig can't leak) - New useFeatures()/useFeature() hook: TanStack Query, 5-min staleTime, reads /api/features - New <FeatureGate feature="..."> component for declarative client gating - New <EnterpriseRequiredEmptyState feature="..."> reusable empty state with auto-generated copy per feature + upgrade CTA - Settings layout: filter Authentication tab by sso feature; hidden on community (avoids dead-end and UI flicker during initial load) - Settings/authentication page: wrap content in FeatureGate; community users see the EnterpriseRequiredEmptyState directly Tests: - 49 unit tests pass (6 new useFeatures + 6 FeatureGate + 1 new community- edition page test + updated route tests + all pre-existing tests) - New E2E spec app/e2e/sso-gating.spec.ts covers community-mode UI + API gating - Enterprise-mode E2E coverage filed as #933 (requires second Playwright worker — substantial global-setup overhaul, out of scope) Drill brief: claude_code_docs/plans/issue-906.md Closes #906 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(auth): address CodeRabbit feedback on #934 Three fixes from the CodeRabbit review: 1. POST + PATCH beforeEach in route.test.ts now pin NEOBOARD_EDITION=enterprise (deterministic across ambient envs; previously could flip to 402 if community env leaked in) 2. Added 402 ENTERPRISE_REQUIRED contract tests for POST, DELETE, PATCH — previously only GET covered the new contract; now full CRUD surface is locked 3. sso-gating.spec.ts: /api/sso-providers test uses page.request.get(...) instead of the global request context. Fixes the CI ECONNRESET seen on E2E shard 4/5 (global request raced with server startup) and matches the authenticated journey set up in the describe block's beforeEach All 28 route tests pass. Local E2E deferred per user direction (Docker teardown cost outweighs benefit for this small fix-up; CI will verify). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(auth): mark FeatureGate + EnterpriseRequiredEmptyState props as readonly Addresses two SonarCloud findings on #934 (rule typescript:S6759, MINOR): - app/src/components/feature-gate.tsx:34 - app/src/components/enterprise-required-empty-state.tsx:78 Per the codebase pattern (e.g. save-template-dialog.tsx, dashboard-picker-dialog.tsx), mark each prop interface field with the `readonly` modifier. 6 feature-gate tests pass. Local E2E deferred to CI (no functional change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(import): unify NeoBoard + NeoDash flow with mapping UI + notes Closes #916. Unblocks #915. ## Server (app/src/app/api/dashboards/import/route.ts) - Accept `connectionMapping` + `skippedConnections` for BOTH formats - NeoDash imports now honor a `neodash-default` placeholder mapping - Cross-tenant safety: mapping validation now scoped to (userId, tenantId) - Response envelope adds `notes: string[]` (additive — existing clients reading only `id` continue to work) - Notes thread through from the converter (chart-type downgrades) plus new import-time notes (skipped connections, unmapped widget counts) ## Converter (app/src/lib/dashboard/neodash-converter.ts) - `convertNeoDash(json, defaultConnectionId?)` — accepts a connection id to stamp on every widget. Falls back to "" (legacy behavior) when omitted. - `convertNeoDashWithNotes` mirrors the signature ## Dialog (app/src/app/(dashboard)/page.tsx) - NeoDash imports synthesize a single placeholder `neodash-default` with type `neo4j` and surface it in the mapping UI (no longer silently uses empty connectionId) - New "Skip" checkbox per mapping row — widgets using a skipped key import with connectionId="" and a note in the result - Empty-targets UX: when no compatible connection exists for the placeholder type, the select is disabled and a helper line offers "Create one" (opens /connections in a new tab) or "Skip" - Post-success view replaces the form: dashboard name + notes list + "Stay here" / "View dashboard" buttons (no longer auto-redirects so users can read import notes carefully) ## Hook (app/src/hooks/use-dashboards.ts) - `ImportDashboardResult extends DashboardDetail` adds `notes: string[]` - `ImportDashboardInput` accepts optional `skippedConnections` ## Tests (app/src/app/api/dashboards/import/__tests__/route.test.ts) - 5 new contract tests: - notes envelope is always present - NeoDash with mapped connection - NeoDash with skipped placeholder → warning note - NeoBoard with skipped → unmapped-widget note - cross-tenant mapping rejected (400) - All 54 unit tests pass ## Out of scope (filed for follow-up) - Inline "Create new connection" affordance inside the import dialog — current "open /connections in new tab" is the fallback flagged in the drill - NeoDash converter content fidelity (params, markdown body) — that's #915 which now has the notes infra it needs to land cleanly ## Drill brief claude_code_docs/plans/issue-916.md Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(e2e): update import E2E specs for new post-success view + NeoDash mapping The PR #935 dialog redesign changed two behaviors that the existing import E2E tests assumed: 1. No auto-redirect after Import — dialog now shows a notes summary with View/Stay buttons; tests must click "View dashboard" before waitForURL 2. NeoDash imports now show a synthesized Neo4j placeholder mapping row instead of skipping the mapping step entirely Fixes 4 failing E2E tests on shards 2/5 and 3/5: - dashboard-portability.spec.ts:52 — NeoBoard format import - dashboard-portability.spec.ts:141 — NeoDash chart-type mapping - dashboard-portability.spec.ts:189 — NeoDash unsupported-type fallback - import-validation.spec.ts:124 — multi-connection mapping happy path For the NeoDash tests, the new placeholder is skipped via the Skip checkbox (these tests assert chart-type behavior, not connection wiring — widgets render regardless of connection presence). Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(import): drop dashboard title from NeoDash placeholder name The synthesized placeholder name was "Neo4j connection (<title>)", which duplicated the dashboard title that's already shown above in the parsed- preview box. In strict-mode E2E selectors this caused dashboard-portability spec line 145 to fail: dialog.getByText("E2E NeoDash Import Test") resolved to 2 elements (the preview header AND the placeholder row). Placeholder name is now just "Neo4j connection" — semantically just as clear (user sees the type "neo4j" beneath it) and avoids the collision. Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(import): preserve NeoDash markdown + auto-generate parameter widgets Closes #915. Three NeoDash converter bugs fixed in one place. ## Bug A — top-level params dropped NeoDash stores dashboard-wide params in `nd.settings.parameters`; NeoBoard has no global params (they're outputs of parameter-select widgets). The converter now: 1. Regex-scans every converted widget query for `$param_<name>` references 2. For each defined param that's referenced → creates a parameter-select widget with inferred type + default value on a NEW "Filters" page (prepended as page 1) 3. For each defined param that's NOT referenced → skip + note 4. For each referenced-but-undefined param → create with no default + warn Type inference: array→multi-select, finite number→number-range, empty string→text, otherwise→select (NeoDash's most common case). Strips the legacy "neodash_" prefix from param names so the generated widget produces `$param_<name>` matching what queries reference (paired with `convertParamSyntax` which already rewrites `$neodash_X` → `$param_X` in queries before scanning). Filter widgets tile 4-per-row at w=3 h=2, connectionId="" (no data). ## Bug B — markdown content dropped NeoDash stored markdown body in `report.query`. Markdown widget reads from `settings.content`. Converter now: - Routes `report.query` into `settings.content` when chartType is markdown - Clears widget.query (markdown is content-only — no query path needed) - Emits per-widget note: 'Imported markdown content for "<title>"' ## Bug C — silent failure mode Uses the existing notes infrastructure from #916 / PR #935 to surface every conversion decision. Notes per the drill (#915 brief): per-param explicit notes so user knows exactly what happened. Acceptable verbosity trade-off — terse summary alternative was considered and rejected. ## Tests 30 new pure-function unit tests cover: - isNeoDashFormat (4) - inferParameterType — all branches (6) - extractParamReferences — happy + edges (5) - Markdown content routing (4) - Filters-page generation (8) - defaultConnectionId behavior (3) Plus all 54 existing tests across the converter / route / dashboard suite continue to pass. Build + type-check green. ## Out of scope (per drill) - Reference detection beyond queries (titles, click-action params, styling rules) — first pass scans queries only - Auto-wiring seed queries for select-typed params — user configures in the editor - Markdown that contains `$param_*` substitutions — NeoDash didn't do inline substitution; literal copy Drill brief: claude_code_docs/plans/issue-915.md Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(import): address CR + Sonar findings on #936 - Extract buildFiltersPage() helper (Sonar S3776: cognitive complexity 20 → 15) - Drop unnecessary type assertion on nd.settings?.parameters (Sonar S4325) - number-range rangeMin = min(default, 0) — supports negative defaults (CR) - Update test fixtures to use $neodash_* syntax so tests exercise the full conversion path instead of bypassing it (CR — 4 tests) - Add original-widget query-rewrite assertion (CR nitpick) - Add explicit negative-default test for rangeMin widening - Fix 2 pre-existing tests at app/src/lib/__tests__/dashboard/ to expect the Filters page at pages[0] (original page now at pages[1] when params are referenced) Local: 2834/2834 tests pass; build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(plugins): graceful settings fallback via safeParseSettings + graph hierarchical Closes #917. ## Bug fix (root) `app/src/plugins/graph/settings.ts:layout` now accepts `"hierarchical"` — the type was already in `component/src/charts/graph-chart.tsx:25` but the Zod enum was missing it, causing widgets to crash on previously-saved hierarchical-layout configs. ## Resilience pattern (cross-cutting) New helper `app/src/lib/plugin/safe-parse-settings.ts`: - Wraps `schema.safeParse` with a fallback to schema defaults - Logs a structured warning via `console.warn` on failure (browser-safe; pino is server-only — bundling it into plugin components blows up webpack with `node:crypto` unhandled scheme) - Re-throws only when the schema ITSELF is broken (schema.parse({}) fails) ## Adoption (mechanical, all 20 plugins) Every plugin component migrated from: const settings = <X>SettingsSchema.parse(raw); to: const settings = safeParseSettings(<X>SettingsSchema, raw, "<plugin-id>"); Includes `single-value` which had a manual safeParse fallback — replaced with the helper for consistency + logging. ## Schema audit Cross-checked Zod enums in all 20 plugin settings against chart-side TS types where the chart exports a named union. Only one drift found: graph layout (the root finding). Other plugins don't export named unions, so the safeParse helper provides defense-in-depth. ## Tests - 9 helper unit tests cover: success, failure with defaults, structured log payload, passthrough preservation, undefined/null, missing fields, broken-schema propagation, pluginId in payload - 2843/2843 total tests pass (+9 new) - Build + type-check green ## Out of scope (per drill) - UI badge on widget header when fallback fires (silent log decided) - Compile-time `satisfies` enforcement of schema ⊆ chart-type (deferred; filed as a possible follow-up if drift recurs) Drill brief: claude_code_docs/plans/issue-917.md Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(plugins): smoke test safeParseSettings adoption across all 20 plugins Adds a single parameterized render test that exercises every plugin's component with deliberately-invalid settings, covering the safeParseSettings call site in each of the 20 plugin component files. Why: SonarCloud new_coverage gate failed on #937 — the 20 mechanical 1-line plugin migrations counted as "new code" with no direct coverage. Plugin components don't have unit tests by convention (they're covered via E2E), but the gate doesn't know that. This test lifts new_coverage above the 80% threshold by exercising each plugin's component once. Each test: - Renders plugin.component with garbage settings via @testing-library/react - Asserts no throw (proves safeParseSettings caught the validation failure and returned defaults instead of crashing) Heavy deps stubbed: @neoboard/components widgets, next/dynamic, the heavier internal components that use TanStack Query (table-renderer, form-widget- renderer, graph-exploration-wrapper). 21 new tests pass (20 plugins + 1 sanity check on the list). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(dashboard): suppress self-save 'updated by' banner on revisit Closes #904. Final Phase 2 PR. ## Root cause `app/src/app/(dashboard)/[id]/page.tsx` (lines 148-160) uses sessionStorage to baseline the dashboard version, then fires a "Dashboard updated by X" banner whenever the refetched server version exceeds the stored baseline. That comparison fires on every SELF-save: after a successful PUT, the server bumps version N → N+1; the refetch sees N+1; sessionStorage still says N; banner fires with the user's own name. Then on revisit the banner triggers again or stays stale. ## Fix Update `useUpdateDashboard.onSuccess` to write the new version to sessionStorage BEFORE invalidating the cache. TanStack Query guarantees onSuccess runs before invalidateQueries' refetch lands, so the baseline is in place by the time the detail page's effect reads it. Other-user saves still trigger the banner correctly — they don't run through this user's mutation onSuccess. ## Defense-in-depth via updatedBy === userId (NOT in this PR) Considered during drill but rejected: would require exposing `dashboard.updatedBy` (user UUID) in the API response, which isn't there today. The primary fix solves the actual race; defense-in-depth is unnecessary for the realistic threat model. ## Tests - Unit (4 new cases on `useUpdateDashboard`): - PUT call shape (mutationFn) - onSuccess writes new version to sessionStorage - onSuccess skips write when result has no version field - onSuccess skips write when version is non-numeric - E2E (`dashboard-states.spec.ts`): full flow — create dashboard, save, navigate away, navigate back, assert no "Dashboard updated by" banner 2868/2868 unit tests pass; build green. Drill brief: claude_code_docs/plans/issue-904.md Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(e2e): fix dashboard-states #904 test — Back goes to view mode, not list CI E2E shard 2/5 failed because the test expected `page.waitForURL(/\/dashboards/)` after clicking "Back", but Back actually navigates to /<id> (view mode), not the dashboards list. View mode is where the version-bump effect runs anyway, so the simpler flow exercises the bug directly: 1. Create dashboard → edit mode (version=1) 2. Save → server bumps to version=2; onSuccess writes 2 to sessionStorage 3. Click Back → /<id> view mode 4. View page's effect: refetch sees version=2, sessionStorage says 2 → NO banner Also added per-test unique dashboard name (timestamp suffix) and cleanup at the end to avoid polluting later tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(devex): fail-fast HMAC, seed-from-host fix, dev DNS warning Closes #907 — promote API_KEY_HMAC_SECRET from optional to required so the app refuses to start without it. Previously API key creation would surface a cryptic runtime error; now we fail fast at startup like ENCRYPTION_KEY and NEXTAUTH_SECRET. CLI's `neoboard env init` now generates this secret alongside ENCRYPTION_KEY so a fresh setup is still one command. Closes #898 — hardcode `localhost` in scripts/seed-demo.mjs. Previously the script honoured NEO4J_HOST/PG_HOST env vars; when seeding ran inside the docker-app container, those resolved to container names (e.g. `db`) which the host-side dev server then couldn't reach. Docker compose publishes the ports to localhost anyway, so the host-form is correct everywhere. Closes #899 — add a dev-only DNS-resolution check that warns about seeded connections whose URIs point to unreachable hosts. Fire-and-forget so startup never blocks; falls back to a no-op outside development. Warns once per affected connection with a concrete fix hint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): type warn mock so app tsc accepts the assignment CI's TypeScript type-check rejected `vi.fn()` assigned to a `(message: string) => void` slot. Use the typed `vi.fn<T>()` overload so the mock satisfies the callable signature while still exposing `.mock`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(dev): redact URI in DNS warn, add tenant scope to diagnostic query Two CodeRabbit findings on the new dev-only DNS checker: - The warning included the full decrypted URI, which can be `scheme://user:password@host/...` — that violates the repo rule "NEVER log decrypted credentials." Print only the parsed hostname. - The diagnostic query selected from `connections` without a tenant filter, violating the multi-tenancy rule that every DB query include one. Scope to `process.env.TENANT_ID ?? "default"`. Test gains a credential-leak guard that fails if any URI-embedded username/password reaches the warn sink. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
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
2 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
Gates the SSO settings page + API by enterprise edition. Builds on the
existing
requireFeature(...)infrastructure rather than the inlineNEOBOARD_EDITION checks. Adds reusable client-side gating primitives
(
useFeatures(),<FeatureGate>,<EnterpriseRequiredEmptyState>) thatfuture enterprise-gated features can reuse.
Drill brief lives at
claude_code_docs/plans/issue-906.md.What changed
Server-side gating
/api/sso-providers(admin CRUD): replaced the inlineif (process.env.NEOBOARD_EDITION !== "enterprise")check withrequireFeature("sso"). Now returns 402ENTERPRISE_REQUIREDinstead of 403 (matches the
EnterpriseRequiredErrormapping inhandleRouteError)./api/auth/sso-providers(public login-page route): short-circuitsto an empty
{ data: [], meta: { enforceSso: false } }response oncommunity before any DB read. Defense-in-depth — even stale
sso_providerrows or env-provider misconfig can't leak.
Client-side gating primitives (reusable)
useFeatures()/useFeature(id)— TanStack Query, 5-min staleTime,reads
/api/features.useFeaturereturnstrue | false | undefined(undefined during initial load so callers can avoid UI flicker).
<FeatureGate feature="...">— declarative wrapper; renderschildren when enabled, fallback otherwise.
hideOnLoadingpropcontrols flicker behaviour.
<EnterpriseRequiredEmptyState feature="...">— reusable emptystate with per-feature copy + upgrade CTA. Future enterprise gates
reuse this directly.
UI changes
settings/layout.tsx): "Authentication" tab isfiltered out on community via
useFeature("sso"). Tab is hidden duringthe initial load to avoid flicker.
settings/authentication/page.tsx):wrapped in
<FeatureGate>with<EnterpriseRequiredEmptyState>asfallback. Community users who type the URL directly land on a
dignified upsell page, not a 404.
Test plan
Unit (vitest) — 49 tests passing
use-features.test.ts— 6 tests (queryFn unwrap, cache config,undefined-while-loading, true/false branches, all-features-false
on community)
feature-gate.test.tsx— 6 tests (enabled/disabled/loading/no-fallbackand hideOnLoading variants)
sso-providers/route.test.ts— community now expects 402ENTERPRISE_REQUIRED with correct error.code (was 403)
auth/sso-providers/route.test.ts— community returns empty arrayAND skips the DB call entirely (asserts
mockDb.selectnot invoked);enterprise still returns rows
settings/authentication/page.test.tsx— new test for communitymode renders the EnterpriseRequiredEmptyState and not the
provider-management UI; existing tests continue to pass (mocked
mockSsoEnabled = trueby default)E2E (Playwright) — community-mode only
New spec:
app/e2e/sso-gating.spec.ts/settings/authenticationshows "Enterprise feature" empty statewith upgrade link
/api/sso-providersreturns 402 ENTERPRISE_REQUIRED/api/auth/sso-providersreturns empty array (withenforceSso: false)Enterprise-mode E2E coverage requires a second Playwright worker
(separate Next.js server with
NEOBOARD_EDITION=enterprise) — substantialglobal-setup overhaul tracked in #933. Out of scope here.
Build + lint
npm run buildpassesnpm run lint— 2 pre-existing errors + 3 warnings in unrelatedfiles (
form-widget-renderer.tsx,graph-exploration-wrapper.tsx,widget-editor-modal.tsx,debounced-text-input.tsx). Noneintroduced by this PR.
Decisions locked from drill
[]on communityrequireFeature("sso")useFeatures()via TanStack Query, 5-min staleTimeRelated issues
requireFeaturepattern)Risk
sso_providerDB rows on communityuseFeaturescache stale across edition flipReviewer notes
The original #906 issue claimed "no enterprise check exists" — that turned
out to be partially incorrect. The Phase 1 audit + drill revealed that
/api/sso-providersalready had an inline check (returning 403 forbidden).This PR refines the issue's scope:
requireFeature("sso")(canonical,consistent — now returns 402 instead of 403)
/api/auth/sso-providersrouteon community
Closes #906
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests