feat(enterprise): SSO OIDC providers — schema, API, auth flow - #702
Merged
Conversation
Add enterprise SSO infrastructure: M1 — Data Model + API: - sso_providers Drizzle schema with encrypted client secrets (AES-256-GCM) - Admin-only CRUD API with tenant isolation and max 5 providers limit - Public listing endpoint for login page SSO buttons - Core extension point system (bootstrapExtensions) that dynamically loads @neoboard/enterprise when NEOBOARD_EDITION=enterprise - Enterprise package as npm workspace with sync-public workflow M2 — OIDC Auth Flow: - Claim-based role mapping (IdP claims → NeoBoard roles, with dot-notation support for nested claims like realm_access.roles) - User provisioning/linking: auto-provision toggle, account linking by email+tenantId, role sync on every login - Provider loader: converts DB rows to Auth.js OIDC provider configs with decrypted secrets Closes #693, closes #694, closes #695, closes #696, closes #697 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds enterprise SSO: Drizzle ssoProviders schema, provider loader/cache/env provider, claim mapping and user provisioning, NextAuth async SSO wiring, admin CRUD and public listing APIs, client hooks and settings UI, extension/enterprise scaffolding, CI/public-mirror sync excluding enterprise/, E2E Keycloak tests, docs, and accessibility tweaks. ChangesEnterprise SSO Feature Implementation
Sequence Diagram(s)sequenceDiagram
participant Browser
participant NextAuth
participant ProviderCache
participant DB
participant Crypto
Browser->>NextAuth: OIDC callback (provider)
NextAuth->>ProviderCache: getCachedSsoProviders(tenantId)
ProviderCache->>DB: loadSsoProviders (if cache miss)
DB->>Crypto: decrypt clientSecretEncrypted
Crypto-->>DB: clientSecret
NextAuth->>DB: provisionOrLinkSsoUser(email, resolvedRole, tenantId)
DB-->>NextAuth: user record / null
NextAuth-->>Browser: accept or reject signIn
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
.github/workflows/ci.yml (1)
131-134: ⚡ Quick winGenerate and upload enterprise coverage for Sonar parity.
Line 132 runs enterprise tests without coverage, and Line 160-164 uploads no enterprise lcov file. This leaves new enterprise code outside Sonar coverage inputs.
Suggested CI diff
- if [ -d enterprise ]; then - cd enterprise && npm run test & + if [ -d enterprise ]; then + cd enterprise && npm run test:coverage & ENT_PID=$! fipath: | app/coverage/lcov.info component/coverage/lcov.info connection/coverage/lcov.info cli/coverage/lcov.info + enterprise/coverage/lcov.infoAs per coding guidelines, ".github/workflows/**: ... check ... that coverage is generated before any SonarQube scan step."
Also applies to: 153-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 131 - 134, The CI currently runs enterprise tests in the enterprise directory with "npm run test" (backgrounded to ENT_PID) but does not generate or upload an lcov file for Sonar; update the enterprise test invocation (the block that cd's into enterprise and sets ENT_PID) to run the coverage-aware test script (e.g., the package.json script that produces lcov, such as "npm run test:coverage" or use nyc/jest CLI to emit lcov) and ensure the subsequent Sonar upload steps (the block that handles uploading lcov for the main repo) also picks up and uploads the enterprise lcov file (add the enterprise lcov path to the upload/sonar input list) so Sonar receives enterprise coverage data.app/src/lib/extensions/extensions.ts (1)
56-63: ⚡ Quick winReturn a read-only registry view from
getExtensions().
getExtensions()exposes the mutable singleton, so any consumer can mutate extension state after bootstrap. Returning a frozen/read-only object prevents accidental runtime drift.🤖 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/lib/extensions/extensions.ts` around lines 56 - 63, getExtensions currently returns the mutable singleton registry allowing callers to mutate extension state; change it to return a read-only view by freezing or exposing an immutable wrapper of the registry before returning. Locate the getExtensions function and instead of returning registry directly, return Object.freeze(copy) or a shallow readonly wrapper of the ExtensionRegistry (or use a read-only interface) so consumers cannot mutate the underlying registry; ensure bootstrapExtensions still initializes the original mutable registry variable internally (registry) while getExtensions returns the immutable/frozen view.app/src/app/api/sso-providers/__tests__/route.test.ts (3)
134-334: ⚡ Quick winAdd explicit negative tests for
canWriteand missingtenantIdbefore mutations.POST/DELETE suites should assert the route rejects these cases and does not hit DB calls. This protects tenant and write-authorization guarantees from regression.
As per coding guidelines,
app/src/**/api/**/*.{ts,tsx}: "ALWAYS enforcecan_writepermission server-side in API routes, not just in UI. JWT tokens must includetenantIdclaim. Validate before ANY DB or API access."Also applies to: 340-392
🤖 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 134 - 334, Tests for POST/DELETE need explicit negative cases asserting the route rejects requests when session.canWrite is false and when session.tenantId is missing, and must verify no DB calls are made; add tests that set mockRequireAdmin to resolve a session with canWrite: false (and another with tenantId: undefined/null) then call POST (and DELETE in the other file) and expect 403/400 as appropriate and that mockDb.select/mockDb.insert/mockDb.delete were not called. Locate the test suites using the POST symbol imported from "../route", the mockRequireAdmin helper used in beforeEach, and the mockDb.* mocks to assert they remain uninvoked for these negative cases.
101-127: ⚡ Quick winStrengthen secret-redaction assertions with fixtures that include encrypted secrets.
Current assertions are non-protective because test rows already omit secret fields. Include
clientSecretEncryptedin mocked DB rows and assert the response excludes it.Diff suggestion
const rows = [ { id: "sso-1", name: "Company SSO", + clientSecretEncrypted: "encrypted:should-not-leak", protocol: "oidc", issuer: "https://idp.example.com",const insertedRow = { id: "new-sso", name: "Company SSO", + clientSecretEncrypted: "encrypted:should-not-leak", protocol: "oidc",As per coding guidelines,
app/src/**: "No credentials logged or stored in DB (AES-256-GCM envelope scheme in use)."Also applies to: 252-279
🤖 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 101 - 127, The test currently mocks DB rows without a secret so the redaction assertion is ineffective; update the mocked rows returned by mockDb.select/makeSelectChain to include a clientSecretEncrypted field (e.g., "encrypted-value") for the SSO row, call GET(makeRequest(...)) as before, and assert that the JSON response's data[0] does NOT have the clientSecretEncrypted property (keep the existing expect(body.data[0]).not.toHaveProperty("clientSecretEncrypted") and add the mocked field to the rows used by makeSelectChain); also apply the same change to the related test block around lines 252-279 to ensure coverage.
64-66: ⚡ Quick winReplace
Promise<any>handler types with concrete route types.Using
anyhere weakens strict-mode checks and bypasses exactly the contract these tests should enforce.Diff suggestion
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - let GET: (req: Request) => Promise<any>; + type RouteModule = typeof import("../route"); + let GET: RouteModule["GET"];- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - let POST: (req: Request) => Promise<any>; + type RouteModule = typeof import("../route"); + let POST: RouteModule["POST"];- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - let DELETE: (req: Request) => Promise<any>; + type RouteModule = typeof import("../route"); + let DELETE: RouteModule["DELETE"];As per coding guidelines,
**/*.{ts,tsx}: "TypeScript strict mode enforced. Noanytype without a comment explaining why."Also applies to: 135-137, 341-343
🤖 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 64 - 66, The test declares request handler variables like GET as (req: Request) => Promise<any>, which uses any and bypasses strict typing; change these to concrete route return types such as Promise<Response> (or Promise<NextResponse> if your handlers return NextResponse) and update any other handler declarations at the other locations noted (the similar declarations around the other ranges) to use the same concrete type; ensure you import the correct Response/NextResponse types and update mocks/expectations accordingly so TypeScript can enforce the route contract.app/src/app/api/auth/sso-providers/__tests__/route.test.ts (2)
41-56: ⚡ Quick winMake the “no secret leak” test assert query projection, not just response shape.
The fixture already contains only
{ id, name }, so secret-field assertions are currently vacuous. Assertdb.selectis called with onlyidandnameprojection to catch regressions.As per coding guidelines,
app/src/**: "No credentials logged or stored in DB (AES-256-GCM envelope scheme in use)."🤖 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/auth/sso-providers/__tests__/route.test.ts` around lines 41 - 56, The test currently only checks the response shape and uses a fixture that already omits secrets, so it doesn't catch regressions where the DB query projects secret columns; update the test to assert that mockDb.select was called with a projection limited to ["id","name"] (or equivalent projection arg) when invoking GET, by spying/asserting on mockDb.select call args (the mock called in this file via mockDb.select and makeSelectChain) rather than relying solely on response properties; keep existing response assertions but add the select-call projection assertion to ensure no secret fields are requested from the DB.
21-23: ⚡ Quick winType
GETwithoutanyto keep strict-mode guarantees in tests.Use the route module’s exported type instead of
Promise<any>.As per coding guidelines,
**/*.{ts,tsx}: "TypeScript strict mode enforced. Noanytype without a comment explaining why."🤖 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/auth/sso-providers/__tests__/route.test.ts` around lines 21 - 23, The test declares the GET variable as Promise<any>, breaking strict mode; change it to use the route module's exported type instead: import the GET handler type from the route module and type the test variable accordingly (e.g., use the exported GET type or typeof GET from the route module) so the variable is declared with the concrete Promise return type instead of any, and update the declaration of GET to use that imported/exported type.
🤖 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 @.github/workflows/sync-public.yml:
- Around line 14-17: Add an explicit least-privilege permissions block to the
GitHub Actions job by updating jobs.sync to include a permissions stanza that
only grants the token scopes required for the mirror sync (e.g., contents: read
or contents: write as applicable, and no other scopes), removing reliance on
repo/org defaults; also ensure no unnecessary permissions are granted and verify
that any steps that require coverage generation run before any SonarQube scan
step and that secrets are not exposed in the job.
In `@app/src/app/api/auth/sso-providers/route.ts`:
- Around line 14-15: Replace the silent fallback to "default" for tenant
identification: remove the "?? 'default'" from the tenantId assignment in
route.ts, validate that process.env.TENANT_ID (or the derived tenant context
used by this route) is present at request start and if missing immediately
return an error response (e.g., 400/401/500 as appropriate) or throw to fail
closed; update any code paths using the tenantId variable (and DB queries that
reference tenant_id) to rely on this validated value so no request proceeds
without an explicit tenant context.
In `@app/src/app/api/sso-providers/route.ts`:
- Around line 83-117: Wrap the duplicate and max-provider logic in a single DB
transaction: start a transaction on db, inside it re-check for an existing
issuer (the current existing query) and re-check provider count (the
allProviders count) using row-level locking if your DB/ORM supports SELECT ...
FOR UPDATE; if count >= MAX_PROVIDERS_PER_TENANT return the CONFLICT, otherwise
perform db.insert(ssoProviders) and commit. Additionally ensure a unique DB
constraint on (tenantId, issuer) so concurrent races are safe and catch
unique-constraint/DB-constraint errors from db.insert to return the same
CONFLICT response for issuer duplicates; reference the existing, allProviders,
ssoProviders, MAX_PROVIDERS_PER_TENANT and db.insert symbols when making the
changes.
- Around line 63-66: The POST and DELETE API handlers call requireAdmin() but
never check the returned canWrite flag, allowing read-only admins to perform
mutations; update both the POST function and the DELETE handler to inspect the
returned value from requireAdmin() (e.g., const { tenantId, canWrite } = await
requireAdmin()) and throw/return a 403 or similar error when canWrite is false
before performing any create/update/delete of SSO provider settings so
server-side enforcement of can_write is guaranteed.
In `@app/src/lib/auth/sso/provision.ts`:
- Around line 38-93: The current provision flow races: two concurrent requests
can both miss the existingUser check and collide on the insert; wrap the insert
(the db.insert(users).values(...) call that assigns newUser) in a try/catch and
on unique-constraint/conflict error re-run the select used earlier (the
db.select(...).from(users).where(and(eq(users.email, email), eq(users.tenantId,
tenantId))) that produces existingUser), then update that re-selected user’s
role/canWrite/lastLoginAt similarly to the existingUser branch and return it;
otherwise, on a successful insert return the created record as before. Ensure
you detect the DB unique-constraint error type coming from your DB client and
only fall back to re-select on that error.
- Around line 53-64: The update currently scopes only by users.id (the
db.update(users).set(...).where(eq(users.id, existingUser.id)) call) which
violates multi-tenancy rules; modify the WHERE predicate to include the tenant
filter as well (e.g., combine eq(users.id, existingUser.id) with an eq on the
tenant column using the same tenant identifier as the existingUser or request
context) so the update is explicitly limited to the correct tenant when updating
role/canWrite/lastLoginAt/name/image.
In `@app/src/lib/extensions/extensions.ts`:
- Around line 37-48: The current broad catch around the dynamic import and
enterprise.register silences all errors; change it so only import failures
indicating the module is missing are handled as a graceful downgrade and any
other error (including errors thrown by enterprise.register) is rethrown or
surfaced. Specifically, keep the dynamic import of moduleName but catch the
import error and check the error identity (e.g., error.code ===
'MODULE_NOT_FOUND' or error.message includes moduleName); if it truly indicates
"not installed", log the community-mode warning, otherwise rethrow the error so
failures in enterprise.register or other runtime errors are not silently
swallowed; ensure you still call enterprise.register(registry) only when the
import succeeded.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 131-134: The CI currently runs enterprise tests in the enterprise
directory with "npm run test" (backgrounded to ENT_PID) but does not generate or
upload an lcov file for Sonar; update the enterprise test invocation (the block
that cd's into enterprise and sets ENT_PID) to run the coverage-aware test
script (e.g., the package.json script that produces lcov, such as "npm run
test:coverage" or use nyc/jest CLI to emit lcov) and ensure the subsequent Sonar
upload steps (the block that handles uploading lcov for the main repo) also
picks up and uploads the enterprise lcov file (add the enterprise lcov path to
the upload/sonar input list) so Sonar receives enterprise coverage data.
In `@app/src/app/api/auth/sso-providers/__tests__/route.test.ts`:
- Around line 41-56: The test currently only checks the response shape and uses
a fixture that already omits secrets, so it doesn't catch regressions where the
DB query projects secret columns; update the test to assert that mockDb.select
was called with a projection limited to ["id","name"] (or equivalent projection
arg) when invoking GET, by spying/asserting on mockDb.select call args (the mock
called in this file via mockDb.select and makeSelectChain) rather than relying
solely on response properties; keep existing response assertions but add the
select-call projection assertion to ensure no secret fields are requested from
the DB.
- Around line 21-23: The test declares the GET variable as Promise<any>,
breaking strict mode; change it to use the route module's exported type instead:
import the GET handler type from the route module and type the test variable
accordingly (e.g., use the exported GET type or typeof GET from the route
module) so the variable is declared with the concrete Promise return type
instead of any, and update the declaration of GET to use that imported/exported
type.
In `@app/src/app/api/sso-providers/__tests__/route.test.ts`:
- Around line 134-334: Tests for POST/DELETE need explicit negative cases
asserting the route rejects requests when session.canWrite is false and when
session.tenantId is missing, and must verify no DB calls are made; add tests
that set mockRequireAdmin to resolve a session with canWrite: false (and another
with tenantId: undefined/null) then call POST (and DELETE in the other file) and
expect 403/400 as appropriate and that mockDb.select/mockDb.insert/mockDb.delete
were not called. Locate the test suites using the POST symbol imported from
"../route", the mockRequireAdmin helper used in beforeEach, and the mockDb.*
mocks to assert they remain uninvoked for these negative cases.
- Around line 101-127: The test currently mocks DB rows without a secret so the
redaction assertion is ineffective; update the mocked rows returned by
mockDb.select/makeSelectChain to include a clientSecretEncrypted field (e.g.,
"encrypted-value") for the SSO row, call GET(makeRequest(...)) as before, and
assert that the JSON response's data[0] does NOT have the clientSecretEncrypted
property (keep the existing
expect(body.data[0]).not.toHaveProperty("clientSecretEncrypted") and add the
mocked field to the rows used by makeSelectChain); also apply the same change to
the related test block around lines 252-279 to ensure coverage.
- Around line 64-66: The test declares request handler variables like GET as
(req: Request) => Promise<any>, which uses any and bypasses strict typing;
change these to concrete route return types such as Promise<Response> (or
Promise<NextResponse> if your handlers return NextResponse) and update any other
handler declarations at the other locations noted (the similar declarations
around the other ranges) to use the same concrete type; ensure you import the
correct Response/NextResponse types and update mocks/expectations accordingly so
TypeScript can enforce the route contract.
In `@app/src/lib/extensions/extensions.ts`:
- Around line 56-63: getExtensions currently returns the mutable singleton
registry allowing callers to mutate extension state; change it to return a
read-only view by freezing or exposing an immutable wrapper of the registry
before returning. Locate the getExtensions function and instead of returning
registry directly, return Object.freeze(copy) or a shallow readonly wrapper of
the ExtensionRegistry (or use a read-only interface) so consumers cannot mutate
the underlying registry; ensure bootstrapExtensions still initializes the
original mutable registry variable internally (registry) while getExtensions
returns the immutable/frozen view.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f672903-5c55-4b8c-99b9-3e7075f3e78e
⛔ Files ignored due to path filters (4)
app/drizzle/migrations/0004_busy_champions.sqlis excluded by!app/drizzle/migrations/**app/drizzle/migrations/meta/0004_snapshot.jsonis excluded by!app/drizzle/migrations/**app/drizzle/migrations/meta/_journal.jsonis excluded by!app/drizzle/migrations/**app/tsconfig.tsbuildinfois excluded by!app/tsconfig.tsbuildinfo
📒 Files selected for processing (22)
.github/sync-public-exclude.github/workflows/ci.yml.github/workflows/sync-public.ymlapp/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/lib/auth/sso/__tests__/claim-mapping.test.tsapp/src/lib/auth/sso/__tests__/provider-loader.test.tsapp/src/lib/auth/sso/__tests__/provision.test.tsapp/src/lib/auth/sso/claim-mapping.tsapp/src/lib/auth/sso/provider-loader.tsapp/src/lib/auth/sso/provision.tsapp/src/lib/db/schema.tsapp/src/lib/extensions/__tests__/extensions.test.tsapp/src/lib/extensions/extensions.tsenterprise/package.jsonenterprise/src/__tests__/register.test.tsenterprise/src/index.tsenterprise/src/register.tsenterprise/tsconfig.jsonpackage.json
Comment on lines
+14
to
+17
| jobs: | ||
| sync: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
Set explicit least-privilege workflow permissions.
This job doesn’t define permissions, so it inherits repo/org defaults. For a mirror sync job, explicitly pin permissions to reduce accidental token overreach.
Suggested patch
on:
push:
branches: [main, dev]
workflow_dispatch:
+permissions:
+ contents: read
+
jobs:
sync:
runs-on: ubuntu-latestAs per coding guidelines "CI workflows: check for secret exposure, unnecessary permissions, and that coverage is generated before any SonarQube scan step."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/sync-public.yml around lines 14 - 17, Add an explicit
least-privilege permissions block to the GitHub Actions job by updating
jobs.sync to include a permissions stanza that only grants the token scopes
required for the mirror sync (e.g., contents: read or contents: write as
applicable, and no other scopes), removing reliance on repo/org defaults; also
ensure no unnecessary permissions are granted and verify that any steps that
require coverage generation run before any SonarQube scan step and that secrets
are not exposed in the job.
Comment on lines
+14
to
+15
| const tenantId = process.env.TENANT_ID ?? "default"; | ||
|
|
There was a problem hiding this comment.
Fail closed when tenant context is missing; avoid fallback to "default".
Line 14 silently routes unresolved requests to the default tenant, which can leak provider metadata across tenants under misconfiguration.
Diff suggestion
- const tenantId = process.env.TENANT_ID ?? "default";
+ const tenantId = process.env.TENANT_ID;
+ if (!tenantId) {
+ throw new Error("TENANT_ID is required for /api/auth/sso-providers");
+ }As per coding guidelines, app/src/**: "Multi-tenancy: every DB query must include tenant_id filter."
🤖 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/auth/sso-providers/route.ts` around lines 14 - 15, Replace
the silent fallback to "default" for tenant identification: remove the "??
'default'" from the tenantId assignment in route.ts, validate that
process.env.TENANT_ID (or the derived tenant context used by this route) is
present at request start and if missing immediately return an error response
(e.g., 400/401/500 as appropriate) or throw to fail closed; update any code
paths using the tenantId variable (and DB queries that reference tenant_id) to
rely on this validated value so no request proceeds without an explicit tenant
context.
Comment on lines
+63
to
+66
| export async function POST(request: Request) { | ||
| try { | ||
| const { tenantId } = await requireAdmin(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate requireAdmin implementation =="
fd 'session.ts' app/src/lib/auth -a
echo "== Inspect requireAdmin for tenantId + canWrite enforcement =="
rg -n -C4 'export\\s+async\\s+function\\s+requireAdmin|canWrite|tenantId' app/src/lib/auth/session.ts
echo "== Confirm mutation handlers only call requireAdmin =="
rg -n -C3 'export async function (POST|DELETE)|requireAdmin\\(' app/src/app/api/sso-providers/route.tsRepository: alfredo1996/neoboard
Length of output: 1985
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect POST handler (lines 63-66) =="
sed -n '60,90p' app/src/app/api/sso-providers/route.ts
echo "== Inspect DELETE handler (lines 152-155) =="
sed -n '150,180p' app/src/app/api/sso-providers/route.ts
echo "== Search for any canWrite checks in the entire file =="
rg -n 'canWrite' app/src/app/api/sso-providers/route.tsRepository: alfredo1996/neoboard
Length of output: 1601
Enforce canWrite permission in both POST and DELETE handlers.
requireAdmin() returns canWrite but handlers don't check it. Read-only admins can mutate SSO provider settings. Add explicit canWrite validation before mutations at lines 63–66 (POST) and 152–155 (DELETE).
Coding guideline requires: "ALWAYS enforce can_write permission server-side in API routes, not just in UI."
🤖 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/route.ts` around lines 63 - 66, The POST and
DELETE API handlers call requireAdmin() but never check the returned canWrite
flag, allowing read-only admins to perform mutations; update both the POST
function and the DELETE handler to inspect the returned value from
requireAdmin() (e.g., const { tenantId, canWrite } = await requireAdmin()) and
throw/return a 403 or similar error when canWrite is false before performing any
create/update/delete of SSO provider settings so server-side enforcement of
can_write is guaranteed.
Comment on lines
+83
to
+117
| // Check for duplicate issuer within tenant | ||
| const existing = await db | ||
| .select({ id: ssoProviders.id }) | ||
| .from(ssoProviders) | ||
| .where( | ||
| and( | ||
| eq(ssoProviders.tenantId, tenantId), | ||
| eq(ssoProviders.issuer, issuer), | ||
| ), | ||
| ); | ||
|
|
||
| if (existing.length > 0) { | ||
| return apiError( | ||
| "CONFLICT", | ||
| "An SSO provider with this issuer already exists", | ||
| ); | ||
| } | ||
|
|
||
| // Check max providers limit | ||
| const allProviders = await db | ||
| .select({ id: ssoProviders.id }) | ||
| .from(ssoProviders) | ||
| .where(eq(ssoProviders.tenantId, tenantId)); | ||
|
|
||
| if (allProviders.length >= MAX_PROVIDERS_PER_TENANT) { | ||
| return apiError( | ||
| "CONFLICT", | ||
| "Maximum of " + | ||
| String(MAX_PROVIDERS_PER_TENANT) + | ||
| " SSO providers per tenant", | ||
| ); | ||
| } | ||
|
|
||
| const [provider] = await db | ||
| .insert(ssoProviders) |
There was a problem hiding this comment.
Duplicate/max-provider checks are non-atomic and race-prone.
The two pre-insert checks can pass concurrently, allowing transient limit breaches or DB-constraint failures instead of clean conflict responses. Make this flow atomic (transaction + deterministic conflict handling).
🤖 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/route.ts` around lines 83 - 117, Wrap the
duplicate and max-provider logic in a single DB transaction: start a transaction
on db, inside it re-check for an existing issuer (the current existing query)
and re-check provider count (the allProviders count) using row-level locking if
your DB/ORM supports SELECT ... FOR UPDATE; if count >= MAX_PROVIDERS_PER_TENANT
return the CONFLICT, otherwise perform db.insert(ssoProviders) and commit.
Additionally ensure a unique DB constraint on (tenantId, issuer) so concurrent
races are safe and catch unique-constraint/DB-constraint errors from db.insert
to return the same CONFLICT response for issuer duplicates; reference the
existing, allProviders, ssoProviders, MAX_PROVIDERS_PER_TENANT and db.insert
symbols when making the changes.
Add admin UI for managing SSO providers: - New Authentication tab in Settings (Shield icon) - Provider list with name, issuer, status badge, default role, SSO enforcement - Add Provider dialog with OIDC config fields, claim mapping UI (IdP claim key → admin/creator/reader values), provisioning toggles, default role - Delete with confirmation (keeps users, disables SSO login) - useSsoProviders TanStack Query hook for CRUD operations - 6 component tests (loading, empty state, provider list, add dialog, delete confirmation, disabled badge) Closes #698 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three fixes:
1. Add sr-only DialogDescription to 9 dialogs that were missing it,
silencing the Radix "Missing Description" console warning. Two
multi-step dialogs use aria-describedby={undefined} to opt out.
Closes #704
2. Fix E2E login flake (95 failures → 0) with two changes:
- Pre-warm /login and auth API routes in global-setup so webpack
compiles them before any test runs (eliminates 3-6s cold start)
- Wait for React 19 hydration (form __reactFiber) in AuthPage.login
before interacting with the form, with 5 retries and 15s timeout
3. Fix breadcrumb Storybook story: add asChild to DropdownMenuTrigger
to avoid nested button HTML. Updates #703
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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)
app/e2e/pages/auth.ts (1)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale JSDoc: still says "Retries up to 3 times".
The loop was bumped to 5 attempts but the JSDoc wasn't updated.
📝 Proposed fix
- * Retries up to 3 times to absorb a known race in the /login page: + * Retries up to 5 times to absorb a known race in the /login page:🤖 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/pages/auth.ts` at line 9, The JSDoc comment that currently reads "Retries up to 3 times to absorb a known race in the /login page" is stale; update that JSDoc to reflect the actual retry count of 5 (e.g., change "3" to "5") in the JSDoc block above the auth helper in app/e2e/pages/auth.ts so it matches the loop that performs 5 attempts.
🧹 Nitpick comments (1)
component/stories/ui/breadcrumb.stories.tsx (1)
64-69: ⚡ Quick winAdd
type="button"to the trigger<button>.Without an explicit
type, a<button>defaults totype="submit", which can trigger unintended form submission if this component is ever used inside a<form>. It's a trivial one-word fix and a consistent good practice.✨ Proposed fix
- <button className="flex items-center gap-1"> + <button type="button" className="flex items-center gap-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 `@component/stories/ui/breadcrumb.stories.tsx` around lines 64 - 69, The trigger button inside DropdownMenuTrigger lacks an explicit type, so add type="button" to the <button> element used as the DropdownMenuTrigger's child to prevent implicit form submission; locate the DropdownMenuTrigger block containing the BreadcrumbEllipsis and update the button element to include type="button".
🤖 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 `@app/e2e/pages/auth.ts`:
- Around line 53-56: The comment claims Promise.all is used but the code awaits
signInButton.click() before waiting for the redirect; change the implementation
to use Promise.all so click() and waitForURL(...) run concurrently (e.g., await
Promise.all([this.page.waitForURL(...), signInButton.click()])), reference the
existing signInButton variable and the subsequent waitForURL call, and remove
the separate try/catch retry block that follows (fold any necessary retry logic
into the new Promise.all flow) so the code and comment match.
In `@app/src/app/`(dashboard)/settings/authentication/__tests__/page.test.tsx:
- Around line 186-197: The test "shows loading spinner when fetching" currently
only asserts static text; update it to assert the actual loading UI when
mockUseSsoProviders returns isLoading: true. After rendering Page (import
"../page"), add an assertion that the loading indicator is present — e.g.
expect(screen.getByRole("progressbar")).toBeInTheDocument() or
expect(screen.getByTestId("loading-spinner")).toBeInTheDocument() — using
whichever loading element your Page component (or its children) renders; keep
the existing mockUseSsoProviders and other assertions.
In `@app/src/hooks/use-sso-providers.ts`:
- Around line 34-43: fetchJson currently calls await res.json() unconditionally
which will throw on 204 No Content or non-JSON responses; change fetchJson (and
the duplicate at lines ~72-76) to first check response status and headers: if
res.status === 204 return an appropriate empty value (e.g. undefined or {} cast
to T), otherwise inspect Content-Type (res.headers.get('content-type')) and only
call res.json() when it indicates JSON; for non-JSON responses call res.text()
and include that text in the thrown Error (or parse as fallback), and preserve
the current behavior of returning body.data when present (return (body?.data ===
undefined ? body : body.data) as T).
---
Outside diff comments:
In `@app/e2e/pages/auth.ts`:
- Line 9: The JSDoc comment that currently reads "Retries up to 3 times to
absorb a known race in the /login page" is stale; update that JSDoc to reflect
the actual retry count of 5 (e.g., change "3" to "5") in the JSDoc block above
the auth helper in app/e2e/pages/auth.ts so it matches the loop that performs 5
attempts.
---
Nitpick comments:
In `@component/stories/ui/breadcrumb.stories.tsx`:
- Around line 64-69: The trigger button inside DropdownMenuTrigger lacks an
explicit type, so add type="button" to the <button> element used as the
DropdownMenuTrigger's child to prevent implicit form submission; locate the
DropdownMenuTrigger block containing the BreadcrumbEllipsis and update the
button element to include type="button".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f83909d-04bf-44c5-9836-16ffa56b7d6a
📒 Files selected for processing (14)
app/e2e/global-setup.tsapp/e2e/pages/auth.tsapp/src/app/(dashboard)/connections/page.tsxapp/src/app/(dashboard)/page.tsxapp/src/app/(dashboard)/settings/authentication/__tests__/page.test.tsxapp/src/app/(dashboard)/settings/authentication/page.tsxapp/src/app/(dashboard)/settings/layout.tsxapp/src/app/(dashboard)/users/page.tsxapp/src/components/dashboard-container.tsxapp/src/components/dashboard-picker-dialog.tsxapp/src/components/save-template-dialog.tsxapp/src/components/widget-editor-modal.tsxapp/src/hooks/use-sso-providers.tscomponent/stories/ui/breadcrumb.stories.tsx
✅ Files skipped from review due to trivial changes (8)
- app/src/components/dashboard-container.tsx
- app/src/app/(dashboard)/users/page.tsx
- app/src/components/save-template-dialog.tsx
- app/src/app/(dashboard)/page.tsx
- app/src/app/(dashboard)/connections/page.tsx
- app/src/components/dashboard-picker-dialog.tsx
- app/src/components/widget-editor-modal.tsx
- app/src/app/(dashboard)/settings/authentication/page.tsx
Comment on lines
+53
to
+56
| // Use Promise.all to click and wait for the auth API call simultaneously. | ||
| // This avoids a race where the redirect happens before waitForURL starts. | ||
| const signInButton = this.page.getByRole("button", { name: "Sign in" }); | ||
| await signInButton.click(); |
There was a problem hiding this comment.
Misleading comment — Promise.all is not used here.
The comment says "Use Promise.all to click and wait … simultaneously", but the code is sequential: click() is awaited first, then waitForURL. Either wrap both in Promise.all as the comment implies, or update the comment to reflect the sequential intent.
📝 Option A — update the comment to match the code
- // Use Promise.all to click and wait for the auth API call simultaneously.
- // This avoids a race where the redirect happens before waitForURL starts.
+ // Click first, then wait for the redirect. waitForURL polls, so a
+ // redirect that happens immediately after click() is still caught.
const signInButton = this.page.getByRole("button", { name: "Sign in" });
await signInButton.click();📝 Option B — use Promise.all to match the comment
- const signInButton = this.page.getByRole("button", { name: "Sign in" });
- await signInButton.click();
+ const signInButton = this.page.getByRole("button", { name: "Sign in" });
+ await Promise.all([
+ signInButton.click(),
+ this.page.waitForURL("/", { timeout: 15_000 }),
+ ]);
+ return;Note: if Option B is chosen, remove the try/catch block that follows (lines 58-70) and fold the retry logic accordingly.
📝 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
| // Use Promise.all to click and wait for the auth API call simultaneously. | |
| // This avoids a race where the redirect happens before waitForURL starts. | |
| const signInButton = this.page.getByRole("button", { name: "Sign in" }); | |
| await signInButton.click(); | |
| // Click first, then wait for the redirect. waitForURL polls, so a | |
| // redirect that happens immediately after click() is still caught. | |
| const signInButton = this.page.getByRole("button", { name: "Sign in" }); | |
| await signInButton.click(); |
🤖 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/pages/auth.ts` around lines 53 - 56, The comment claims Promise.all
is used but the code awaits signInButton.click() before waiting for the
redirect; change the implementation to use Promise.all so click() and
waitForURL(...) run concurrently (e.g., await
Promise.all([this.page.waitForURL(...), signInButton.click()])), reference the
existing signInButton variable and the subsequent waitForURL call, and remove
the separate try/catch retry block that follows (fold any necessary retry logic
into the new Promise.all flow) so the code and comment match.
Comment on lines
+186
to
+197
| it("shows loading spinner when fetching", async () => { | ||
| mockUseSsoProviders.mockReturnValue({ | ||
| data: undefined, | ||
| isLoading: true, | ||
| }); | ||
|
|
||
| const { default: Page } = await import("../page"); | ||
| render(<Page />); | ||
|
|
||
| expect(screen.getByText("Authentication")).toBeInTheDocument(); | ||
| expect(screen.queryByTestId("empty-state")).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Loading test doesn’t verify a loading indicator.
The case name says “shows loading spinner,” but it only asserts static text. Please assert an actual loading UI (e.g., progressbar/spinner test id) so this test can catch regressions.
Suggested test tightening
it("shows loading spinner when fetching", async () => {
mockUseSsoProviders.mockReturnValue({
data: undefined,
isLoading: true,
});
const { default: Page } = await import("../page");
render(<Page />);
- expect(screen.getByText("Authentication")).toBeInTheDocument();
- expect(screen.queryByTestId("empty-state")).not.toBeInTheDocument();
+ expect(screen.getByText("Authentication")).toBeInTheDocument();
+ expect(screen.getByRole("progressbar")).toBeInTheDocument();
+ expect(screen.queryByTestId("empty-state")).not.toBeInTheDocument();
});📝 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
| it("shows loading spinner when fetching", async () => { | |
| mockUseSsoProviders.mockReturnValue({ | |
| data: undefined, | |
| isLoading: true, | |
| }); | |
| const { default: Page } = await import("../page"); | |
| render(<Page />); | |
| expect(screen.getByText("Authentication")).toBeInTheDocument(); | |
| expect(screen.queryByTestId("empty-state")).not.toBeInTheDocument(); | |
| }); | |
| it("shows loading spinner when fetching", async () => { | |
| mockUseSsoProviders.mockReturnValue({ | |
| data: undefined, | |
| isLoading: true, | |
| }); | |
| const { default: Page } = await import("../page"); | |
| render(<Page />); | |
| expect(screen.getByText("Authentication")).toBeInTheDocument(); | |
| expect(screen.getByRole("progressbar")).toBeInTheDocument(); | |
| expect(screen.queryByTestId("empty-state")).not.toBeInTheDocument(); | |
| }); |
🤖 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/`(dashboard)/settings/authentication/__tests__/page.test.tsx
around lines 186 - 197, The test "shows loading spinner when fetching" currently
only asserts static text; update it to assert the actual loading UI when
mockUseSsoProviders returns isLoading: true. After rendering Page (import
"../page"), add an assertion that the loading indicator is present — e.g.
expect(screen.getByRole("progressbar")).toBeInTheDocument() or
expect(screen.getByTestId("loading-spinner")).toBeInTheDocument() — using
whichever loading element your Page component (or its children) renders; keep
the existing mockUseSsoProviders and other assertions.
…s (M4) Wire SSO providers into Auth.js and add SSO buttons to the login page: - Refactor NextAuth to lazy config (async function) so OIDC providers are loaded from the DB on each auth flow with 60s in-memory cache - Add signIn callback: detect SSO login, resolve role from IdP claims, provision/link user, populate JWT with SSO user data - Login page: fetch enabled SSO providers, render "Sign in with [Name]" buttons above password form with divider - SSO enforcement: when enforceSso is set, hide password form; admins can bypass via ?password=true query param - Public endpoint returns enforceSso flag in response meta - Provider cache with TTL (6 tests), config test updated for lazy init Closes #699, closes #700 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/src/app/(auth)/login/page.tsx (1)
141-172: ⚡ Quick winPassword form flashes briefly when SSO is enforced.
ssoEnforcedinitialises tofalse, soshowPasswordFormistrueon every first render. WhenenforceSsoistrue, the form is visible until the fetch to/api/auth/sso-providerssettles, then it disappears — a jarring UX.Adding an
isLoadinggate prevents the premature render:♻️ Proposed fix
+ const [ssoLoading, setSsoLoading] = useState(true); useEffect(() => { fetch("/api/auth/sso-providers") .then((r) => r.json()) .then((body) => { const providers = body?.data ?? []; setSsoProviders(providers); if (body?.meta?.enforceSso) { setSsoEnforced(true); } }) - .catch(() => {}); + .catch(() => {}) + .finally(() => setSsoLoading(false)); }, []); - const showPasswordForm = - !ssoEnforced || forcePassword || ssoProviders.length === 0; + const showPasswordForm = + !ssoLoading && (!ssoEnforced || forcePassword || ssoProviders.length === 0);🤖 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/`(auth)/login/page.tsx around lines 141 - 172, The password form flashes because ssoEnforced starts false and showPasswordForm is computed before the fetch completes; update LoginPageContent to track loading (e.g., add an isSsoLoading state) and keep the password form hidden while the SSO fetch is pending: set isSsoLoading true before calling fetch("/api/auth/sso-providers"), set it false in both then and catch, and change the showPasswordForm condition to include !isSsoLoading (e.g., showPasswordForm = (!ssoEnforced || forcePassword || ssoProviders.length === 0) && !isSsoLoading), using the existing setSsoEnforced and setSsoProviders to update values when the request resolves.app/src/lib/auth/sso/provider-cache.ts (1)
17-34: ⚡ Quick winConcurrent cache misses trigger a thundering herd of
loadSsoProviderscalls.Auth.js v5 lazy initialization invokes the config function on each auth request, so multiple simultaneous requests hitting an expired or cold cache for the same
tenantIdwill all callloadSsoProvidersconcurrently before any of them has written a result back. Deduplicate by caching the in-flightPromisedirectly:♻️ Proposed fix — in-flight Promise deduplication
-interface CacheEntry { +interface CacheEntry { providers: LoadedSsoProvider[]; expiresAt: number; } -const cache = new Map<string, CacheEntry>(); +const cache = new Map<string, CacheEntry>(); +const inFlight = new Map<string, Promise<LoadedSsoProvider[]>>(); export async function getCachedSsoProviders( tenantId: string, ): Promise<LoadedSsoProvider[]> { const now = Date.now(); const cached = cache.get(tenantId); if (cached && cached.expiresAt > now) { return cached.providers; } + const existing = inFlight.get(tenantId); + if (existing) return existing; + const promise = (async () => { try { const providers = await loadSsoProviders(tenantId); cache.set(tenantId, { providers, expiresAt: now + CACHE_TTL_MS }); return providers; } catch { return []; + } finally { + inFlight.delete(tenantId); } + })(); + inFlight.set(tenantId, promise); + return promise; }🤖 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/lib/auth/sso/provider-cache.ts` around lines 17 - 34, getCachedSsoProviders currently causes a thundering herd because multiple concurrent calls on a cache miss all invoke loadSsoProviders; fix by storing the in-flight Promise in the same cache key so subsequent callers await the same Promise: on cache miss set cache.set(tenantId, { promise: loadPromise }) where loadPromise = loadSsoProviders(tenantId).then(result => { cache.set(tenantId, { providers: result, expiresAt: Date.now()+CACHE_TTL_MS }); return result }).catch(err => { cache.delete(tenantId); throw err }), then return await loadPromise; update references to loadSsoProviders, cache, CACHE_TTL_MS and getCachedSsoProviders accordingly and ensure failed promises remove the cache entry.
🤖 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 `@app/src/lib/auth/__tests__/config.test.ts`:
- Around line 116-119: Replace the fragile fixed sleep in beforeAll with a
polling wait that checks the NextAuth mock has finished resolving by verifying
callbacks.jwt and callbacks.session are set to functions; repeatedly check (with
a short delay and a global timeout) until both typeof callbacks.jwt ===
'function' and typeof callbacks.session === 'function' before continuing. Update
the beforeAll setup that currently awaits setTimeout to instead poll the
NextAuth mock's resolve state (or inspect the exported callbacks object from the
mock) and throw a clear timeout error if the wait exceeds a reasonable limit.
In `@app/src/lib/auth/config.ts`:
- Around line 102-110: The code currently returns true when account.provider
startsWith("sso-") but no matching providerConfig is found; change this to
return false so that authentication fails fast instead of creating a partial
session. Update the branch in the function that checks account.provider and the
ssoProviders lookup (the block that computes providerConfig from ssoProviders
and currently does "if (!providerConfig) return true") to return false and
ensure any surrounding logic expecting a boolean handles the denied case
consistently.
- Around line 60-65: The query that selects a user by email must also filter by
tenantId to enforce multi-tenancy: update the
db.select(...).from(users).where(...) call (the block that currently uses
eq(users.email, parsed.data.email)) to include an AND condition combining
eq(users.email, parsed.data.email) and eq(users.tenant_id, tenantId) (use
drizzle-orm's and helper). Also add the missing import for and from
'drizzle-orm' alongside the existing eq import to ensure the combined predicate
compiles and the query always scopes to the current tenant.
- Around line 163-177: The DB re-fetch in the JWT callback (the
db.select(...).from(users).where(eq(users.id, token.id as string)).limit(1)
block) is missing the tenant filter and thus violates multi-tenancy rules;
update the where clause to also require eq(users.tenantId, token.tenantId)
(i.e., combine the existing id predicate with a tenantId predicate using the
query builder's AND semantics) so the lookup uses both token.id and
token.tenantId from the closure.
In `@app/src/lib/auth/sso/__tests__/provider-cache.test.ts`:
- Around line 60-76: The test sets fake timers with vi.useFakeTimers() but calls
vi.useRealTimers() only inline, risking leaked fake timers if the test fails;
move timer cleanup into a global afterEach in this test file so real timers are
restored regardless of failures — add an afterEach hook that calls
vi.useRealTimers() (and optionally vi.clearAllTimers()) in
app/src/lib/auth/sso/__tests__/provider-cache.test.ts to ensure
getCachedSsoProviders tests and mockLoadSsoProviders TTL assertions run with
real timers restored.
---
Nitpick comments:
In `@app/src/app/`(auth)/login/page.tsx:
- Around line 141-172: The password form flashes because ssoEnforced starts
false and showPasswordForm is computed before the fetch completes; update
LoginPageContent to track loading (e.g., add an isSsoLoading state) and keep the
password form hidden while the SSO fetch is pending: set isSsoLoading true
before calling fetch("/api/auth/sso-providers"), set it false in both then and
catch, and change the showPasswordForm condition to include !isSsoLoading (e.g.,
showPasswordForm = (!ssoEnforced || forcePassword || ssoProviders.length === 0)
&& !isSsoLoading), using the existing setSsoEnforced and setSsoProviders to
update values when the request resolves.
In `@app/src/lib/auth/sso/provider-cache.ts`:
- Around line 17-34: getCachedSsoProviders currently causes a thundering herd
because multiple concurrent calls on a cache miss all invoke loadSsoProviders;
fix by storing the in-flight Promise in the same cache key so subsequent callers
await the same Promise: on cache miss set cache.set(tenantId, { promise:
loadPromise }) where loadPromise = loadSsoProviders(tenantId).then(result => {
cache.set(tenantId, { providers: result, expiresAt: Date.now()+CACHE_TTL_MS });
return result }).catch(err => { cache.delete(tenantId); throw err }), then
return await loadPromise; update references to loadSsoProviders, cache,
CACHE_TTL_MS and getCachedSsoProviders accordingly and ensure failed promises
remove the cache entry.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a09ebd6-e418-4879-be66-ab02e71d8b16
📒 Files selected for processing (6)
app/src/app/(auth)/login/page.tsxapp/src/app/api/auth/sso-providers/route.tsapp/src/lib/auth/__tests__/config.test.tsapp/src/lib/auth/config.tsapp/src/lib/auth/sso/__tests__/provider-cache.test.tsapp/src/lib/auth/sso/provider-cache.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/app/api/auth/sso-providers/route.ts
Comment on lines
+116
to
+119
| beforeAll(async () => { | ||
| // Wait for the NextAuth mock's resolve() to complete | ||
| await new Promise((r) => setTimeout(r, 50)); | ||
| }); |
There was a problem hiding this comment.
setTimeout(r, 50) is a fragile synchronization point.
The resolve() call on line 31 is fire-and-forget, so callbacks.jwt/session are populated asynchronously. A 50 ms hard timeout works on fast machines but can silently leave callbacks as null under CI load, causing every test in the file to throw TypeError: callbacks.jwt is not a function.
🛡️ Proposed fix — poll until resolved
-beforeAll(async () => {
- // Wait for the NextAuth mock's resolve() to complete
- await new Promise((r) => setTimeout(r, 50));
-});
+beforeAll(async () => {
+ // Poll until the async mock has populated the callbacks
+ const deadline = Date.now() + 2_000;
+ while (callbacks.jwt === null) {
+ if (Date.now() > deadline) throw new Error("NextAuth mock never resolved");
+ await new Promise((r) => setTimeout(r, 10));
+ }
+});📝 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
| beforeAll(async () => { | |
| // Wait for the NextAuth mock's resolve() to complete | |
| await new Promise((r) => setTimeout(r, 50)); | |
| }); | |
| beforeAll(async () => { | |
| // Poll until the async mock has populated the callbacks | |
| const deadline = Date.now() + 2_000; | |
| while (callbacks.jwt === null) { | |
| if (Date.now() > deadline) throw new Error("NextAuth mock never resolved"); | |
| await new Promise((r) => setTimeout(r, 10)); | |
| } | |
| }); |
🤖 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/lib/auth/__tests__/config.test.ts` around lines 116 - 119, Replace
the fragile fixed sleep in beforeAll with a polling wait that checks the
NextAuth mock has finished resolving by verifying callbacks.jwt and
callbacks.session are set to functions; repeatedly check (with a short delay and
a global timeout) until both typeof callbacks.jwt === 'function' and typeof
callbacks.session === 'function' before continuing. Update the beforeAll setup
that currently awaits setTimeout to instead poll the NextAuth mock's resolve
state (or inspect the exported callbacks object from the mock) and throw a clear
timeout error if the wait exceeds a reasonable limit.
Comment on lines
+60
to
+76
| it("re-fetches after cache expires", async () => { | ||
| vi.useFakeTimers(); | ||
| const providers = [{ id: "sso-1", name: "Test" }]; | ||
| mockLoadSsoProviders.mockResolvedValue(providers); | ||
|
|
||
| const { getCachedSsoProviders } = await import("../provider-cache"); | ||
| await getCachedSsoProviders("default"); | ||
| expect(mockLoadSsoProviders).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Advance past the 60s TTL | ||
| vi.advanceTimersByTime(61_000); | ||
|
|
||
| await getCachedSsoProviders("default"); | ||
| expect(mockLoadSsoProviders).toHaveBeenCalledTimes(2); | ||
|
|
||
| vi.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
Fake timers not guaranteed to be restored if the test throws early.
vi.useRealTimers() at line 75 is only reached if all assertions pass. A test failure before that point leaves fake timers active for the remaining tests, corrupting Date.now() in TTL comparisons.
🛡️ Proposed fix — move timer cleanup to `afterEach`
+ afterEach(() => {
+ vi.useRealTimers();
+ });
+
it("re-fetches after cache expires", async () => {
vi.useFakeTimers();
const providers = [{ id: "sso-1", name: "Test" }];
mockLoadSsoProviders.mockResolvedValue(providers);
const { getCachedSsoProviders } = await import("../provider-cache");
await getCachedSsoProviders("default");
expect(mockLoadSsoProviders).toHaveBeenCalledTimes(1);
vi.advanceTimersByTime(61_000);
await getCachedSsoProviders("default");
expect(mockLoadSsoProviders).toHaveBeenCalledTimes(2);
-
- vi.useRealTimers();
});🤖 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/lib/auth/sso/__tests__/provider-cache.test.ts` around lines 60 - 76,
The test sets fake timers with vi.useFakeTimers() but calls vi.useRealTimers()
only inline, risking leaked fake timers if the test fails; move timer cleanup
into a global afterEach in this test file so real timers are restored regardless
of failures — add an afterEach hook that calls vi.useRealTimers() (and
optionally vi.clearAllTimers()) in
app/src/lib/auth/sso/__tests__/provider-cache.test.ts to ensure
getCachedSsoProviders tests and mockLoadSsoProviders TTL assertions run with
real timers restored.
Full OIDC E2E tests against a real Keycloak IdP: - Keycloak container added to global-setup (start alongside PG/Neo4j) - Realm JSON with test users in groups (admins, editors, viewers) - SSO provider seeded in sso_providers table with encrypted secret - 5 E2E test scenarios: SSO button visible, redirect to Keycloak, full login with auto-provision, admin role via claim mapping, provider visible in Settings > Authentication Also fixes: - signIn callback simplified: let DrizzleAdapter handle user creation, signIn only checks auto-provision and syncs role from claims - allowDangerousEmailAccountLinking on SSO providers for account linking - callbackUrl: "/" on SSO signIn to redirect to dashboard after login - AuthPage.login uses exact match for "Sign in" button (avoids matching "Sign in with [Provider]" SSO buttons) Closes #701 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add comprehensive SSO documentation: - New guides/sso.mdx with setup instructions, role mapping, account linking, troubleshooting, and provider deletion behavior - Full configuration examples for Okta, Azure AD/Entra, Keycloak, and Google Workspace - Add Core/Enterprise badges to all guide pages via Starlight frontmatter badges - Fix enterprise variables in configuration.mdx (was SSO_ENABLED, now correctly references NEOBOARD_EDITION=enterprise) - Add SSO link to guides index page Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (5)
app/src/lib/auth/sso/provider-loader.ts (2)
60-78: 💤 Low valueOne bad secret takes down the whole tenant's SSO.
decrypt(row.clientSecretEncrypted)runs inside.map()with no guard. A single row with a corrupted ciphertext, wrong tag, or staleENCRYPTION_KEYwill throw and prevent every other enabled provider from loading. Consider catching per-row and logging the failure (without the secret, of course), then filtering the bad provider out so the rest still work.🤖 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/lib/auth/sso/provider-loader.ts` around lines 60 - 78, The current rows.map(...) calls decrypt(row.clientSecretEncrypted) inline so a decrypt error for any row will throw and abort loading all providers; modify the mapping logic (the block that builds each provider object in provider-loader.ts) to catch decryption errors per-row (wrap the decrypt call in a try/catch), log a concise error referencing the provider id/name (but never include the secret or ciphertext), and skip/filter out that failing row so only successfully decrypted providers are returned; ensure the returned objects still include fields like id, name, type, issuer, clientId, authorization, allowDangerousEmailAccountLinking and metadata unchanged for valid rows.
22-31: ⚡ Quick win
allowDangerousEmailAccountLinkingis set but not declared onLoadedSsoProvider.The mapped object emits
allowDangerousEmailAccountLinking: trueon line 71, but the interface (lines 22-31) doesn't list it. The flag survives at runtime (good — Auth.js will see it), but consumers ofLoadedSsoProvider(provider-cache.ts,config.ts) can't access it through the type. Add it to the interface so the contract matches the runtime shape.♻️ Proposed fix
export interface LoadedSsoProvider { id: string; name: string; type: "oidc"; issuer: string; clientId: string; clientSecret: string; authorization: { params: { scope: string } }; + allowDangerousEmailAccountLinking: boolean; metadata: SsoProviderMetadata; }Also applies to: 60-78
🤖 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/lib/auth/sso/provider-loader.ts` around lines 22 - 31, LoadedSsoProvider is missing the allowDangerousEmailAccountLinking property that the loader sets at runtime; update the LoadedSsoProvider interface to include allowDangerousEmailAccountLinking: boolean so the type matches the actual object shape used by provider-cache.ts and config.ts (also adjust any related mapped returns in the provider loader to satisfy the new property type).app/e2e/global-teardown.ts (1)
66-74: 💤 Low valueTidy the temp-file cleanup with
fs.rmSync({ force: true }).Three try/catches with empty catch blocks read noisier than necessary.
fs.rmSyncwithforce: trueswallows ENOENT natively.♻️ Proposed fix
- // Clean up temp files - try { - fs.unlinkSync(STATE_FILE); - } catch {} - try { - fs.unlinkSync(SERVER_PID_FILE); - } catch {} - try { - fs.unlinkSync(ENV_FILE); - } catch {} + // Clean up temp files (force: true ignores ENOENT). + fs.rmSync(STATE_FILE, { force: true }); + fs.rmSync(SERVER_PID_FILE, { force: true }); + fs.rmSync(ENV_FILE, { force: true });🤖 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/global-teardown.ts` around lines 66 - 74, Replace the three try/catch cleanup blocks that call fs.unlinkSync(STATE_FILE), fs.unlinkSync(SERVER_PID_FILE), and fs.unlinkSync(ENV_FILE) with calls to fs.rmSync(..., { force: true }) for each file (use the same STATE_FILE, SERVER_PID_FILE, and ENV_FILE symbols) so missing files are ignored and the empty catch blocks can be removed.app/e2e/global-setup.ts (1)
279-285: 💤 Low valueReuse
databaseUrlinstead of rebuilding the same string.Line 265 already computes the exact connection string passed here. Pass
databaseUrldirectly to keep one source of truth and avoid drift if the URL ever changes (e.g., adding sslmode).♻️ Proposed fix
// ── Seed SSO provider pointing at the Keycloak test container ────────── console.log("⏳ Seeding SSO provider for Keycloak..."); - await seedSsoProvider( - `postgresql://neoboard:neoboard@${pgHost}:${pgPort}/neoboard`, - keycloakPort, - ); + await seedSsoProvider(databaseUrl, keycloakPort); console.log("✅ SSO provider seeded");🤖 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/global-setup.ts` around lines 279 - 285, The code rebuilds the Postgres connection string when calling seedSsoProvider instead of reusing the previously computed databaseUrl; update the call to seedSsoProvider to pass the existing databaseUrl variable (the one computed on line 265) instead of reconstructing `postgresql://neoboard:neoboard@${pgHost}:${pgPort}/neoboard`, so seedSsoProvider(databaseUrl, keycloakPort) is used and the connection string stays single-source-of-truth.app/e2e/sso.spec.ts (1)
124-128: 💤 Low value
getByText("Enabled")is too broad to anchor the provider row.This will match any element rendering the word "Enabled" on the page (status pills, headings, toggle labels). Scope the assertion to the provider's row/card to keep the test honest if the Settings page evolves.
🤖 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.spec.ts` around lines 124 - 128, The assertion using page.getByText("Enabled") is too broad; scope it to the same provider row/card as the "Keycloak Test" text so it only checks the status for that provider. Locate the provider container via the existing marker (the element matching page.getByText("Keycloak Test") or a row/card role containing that text, e.g., a row/card locator for the provider) and then assert that the "Enabled" text is visible within that container (use the container's locator and call its getByText or locator(...).getByText("Enabled") before toBeVisible). Update the test in sso.spec.ts to use the scoped locator rather than a global getByText("Enabled").
🤖 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.
Nitpick comments:
In `@app/e2e/global-setup.ts`:
- Around line 279-285: The code rebuilds the Postgres connection string when
calling seedSsoProvider instead of reusing the previously computed databaseUrl;
update the call to seedSsoProvider to pass the existing databaseUrl variable
(the one computed on line 265) instead of reconstructing
`postgresql://neoboard:neoboard@${pgHost}:${pgPort}/neoboard`, so
seedSsoProvider(databaseUrl, keycloakPort) is used and the connection string
stays single-source-of-truth.
In `@app/e2e/global-teardown.ts`:
- Around line 66-74: Replace the three try/catch cleanup blocks that call
fs.unlinkSync(STATE_FILE), fs.unlinkSync(SERVER_PID_FILE), and
fs.unlinkSync(ENV_FILE) with calls to fs.rmSync(..., { force: true }) for each
file (use the same STATE_FILE, SERVER_PID_FILE, and ENV_FILE symbols) so missing
files are ignored and the empty catch blocks can be removed.
In `@app/e2e/sso.spec.ts`:
- Around line 124-128: The assertion using page.getByText("Enabled") is too
broad; scope it to the same provider row/card as the "Keycloak Test" text so it
only checks the status for that provider. Locate the provider container via the
existing marker (the element matching page.getByText("Keycloak Test") or a
row/card role containing that text, e.g., a row/card locator for the provider)
and then assert that the "Enabled" text is visible within that container (use
the container's locator and call its getByText or
locator(...).getByText("Enabled") before toBeVisible). Update the test in
sso.spec.ts to use the scoped locator rather than a global getByText("Enabled").
In `@app/src/lib/auth/sso/provider-loader.ts`:
- Around line 60-78: The current rows.map(...) calls
decrypt(row.clientSecretEncrypted) inline so a decrypt error for any row will
throw and abort loading all providers; modify the mapping logic (the block that
builds each provider object in provider-loader.ts) to catch decryption errors
per-row (wrap the decrypt call in a try/catch), log a concise error referencing
the provider id/name (but never include the secret or ciphertext), and
skip/filter out that failing row so only successfully decrypted providers are
returned; ensure the returned objects still include fields like id, name, type,
issuer, clientId, authorization, allowDangerousEmailAccountLinking and metadata
unchanged for valid rows.
- Around line 22-31: LoadedSsoProvider is missing the
allowDangerousEmailAccountLinking property that the loader sets at runtime;
update the LoadedSsoProvider interface to include
allowDangerousEmailAccountLinking: boolean so the type matches the actual object
shape used by provider-cache.ts and config.ts (also adjust any related mapped
returns in the provider loader to satisfy the new property type).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c244b8db-5fb3-4dd8-82b4-6206549079b5
⛔ Files ignored due to path filters (1)
docker/keycloak/neoboard-test-realm.jsonis excluded by!docker/**
📒 Files selected for processing (7)
app/e2e/global-setup.tsapp/e2e/global-teardown.tsapp/e2e/pages/auth.tsapp/e2e/sso.spec.tsapp/src/app/(auth)/login/page.tsxapp/src/lib/auth/config.tsapp/src/lib/auth/sso/provider-loader.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/app/(auth)/login/page.tsx
- app/e2e/pages/auth.ts
- app/src/lib/auth/config.ts
Add env var support for SSO as the primary config method (Option C): - New env-provider.ts: reads OIDC_* env vars, returns LoadedSsoProvider when OIDC_ISSUER + OIDC_CLIENT_ID + OIDC_CLIENT_SECRET are all set - Works in community edition — no NEOBOARD_EDITION=enterprise needed - Env provider merges with DB providers (env first, DB adds on top) - Public /api/auth/sso-providers endpoint includes env provider - 14 unit tests for env-provider, 3 new cache merge tests - Docs updated: env vars as primary method, Docker Compose example, Admin UI moved to "Multi-Provider" subsection - Configuration docs: OIDC vars in the variables table Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allow multiple IdP groups to map to the same NeoBoard role using comma-separated values: OIDC_ADMIN_VALUE=devops,platform-team,it-ops OIDC_CREATOR_VALUE=engineering,data-team Each mapping value is split by comma and trimmed. If any of the user's claim values matches any target, the role is assigned. Priority order (admin > creator > reader) preserved. Backwards compatible — single values without commas work as before. 6 new tests added (19 total for claim mapping). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update SSO guide to show multi-group role mapping: - Variable table notes comma-separated support - Role Mapping section with env var examples - Docker Compose example uses comma-separated groups Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SSO (both env vars and Admin UI) now requires NEOBOARD_EDITION=enterprise. loadEnvSsoProvider() returns null unless the enterprise edition is active. Updated all tests and docs to reflect the requirement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- .env.example: remove "works in community edition", add NEOBOARD_EDITION=enterprise to required vars - configuration.mdx: note enterprise requirement in SSO section - sso.mdx: add NEOBOARD_EDITION to prerequisites list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Major docs reorganization — from 6 generic sections to 8 topic-focused sections: Structure changes: - NEW Authentication/ — Password Login, SSO, API Keys, Roles & Permissions - NEW Dashboards/ — First Dashboard, Widgets, Parameters, Overview - NEW Connections/ — Connecting Databases, Connectors, Query Safety - NEW Administration/ — Managing Users, Multi-tenancy - REMOVED Concepts/ — merged into relevant sections - REMOVED User Guides/ — split across Authentication, Dashboards, Connections - KEPT Getting Started/, Chart Types/, CLI/, Developer Guide/ New pages: - authentication/password-login.mdx — session management, rate limiting, password requirements, forced password change, registration control - authentication/roles.mdx — role comparison table, write permissions, SSO role assignment Developer docs (#708): - Mermaid architecture diagram (packages + dependencies) - "Where Does Code Live?" table (feature → package → directory) - "Common Tasks" section (add API route, chart type, setting, DB table) - Newcomer-friendly numbered getting-started flow on index page All cross-references updated to new paths. Closes #705, closes #708 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@app/src/lib/auth/sso/env-provider.ts`:
- Line 49: The current assignment for defaultRole in env-provider.ts forcibly
casts process.env.OIDC_DEFAULT_ROLE to UserRole which can allow invalid strings;
change the defaultRole logic to read the raw env string, validate it against the
known UserRole values (e.g. the UserRole enum or allowedRoles array), and only
use the env value if it matches an allowed role, otherwise fall back to
"creator"; add a small helper like isValidUserRole or reuse the UserRole enum to
perform the check and then assign defaultRole accordingly.
In `@app/src/lib/auth/sso/provider-cache.ts`:
- Around line 33-41: The code currently caches fallback results when
loadSsoProviders(tenantId) throws, causing env-only or empty dbProviders to be
stored via cache.set(tenantId, { providers, expiresAt: now + CACHE_TTL_MS }); to
fix, do not call cache.set when the DB load failed: detect the error case (the
catch block where dbProviders was not populated or a thrown error occurred) and
only perform cache.set when loadSsoProviders succeeded (i.e., when dbProviders
is valid); keep using envProvider merging logic (envProvider ? [envProvider,
...dbProviders] : dbProviders) but move the cache write into the try path or add
a boolean flag like dbLoaded to gate cache.set, leaving tenantId, CACHE_TTL_MS,
and envProvider usage unchanged.
In `@docs/.astro/content.d.ts`:
- Around line 91-96: The generated types in docs/.astro/content.d.ts reference
LiveLoader, LiveDataCollectionResult, and LiveDataEntryResult (see
getLiveCollection signature), but the docs workspace declares Astro 5.3.0 which
lacks that API; update the docs workspace Astro dependency in docs/package.json
to at least 6.0.0 (or 5.10.0+ if you intend to enable experimental flags), run
package install to update lockfiles, then regenerate the docs types by running
the docs build/dev command (so content.d.ts is recreated against the upgraded
Astro). Ensure the upgrade affects the devDependencies used to produce
docs/.astro/content.d.ts and verify the getLiveCollection and related type
signatures compile cleanly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0db0437-913d-44fd-acad-ee73f624e003
📒 Files selected for processing (40)
.env.exampleapp/src/app/api/auth/sso-providers/route.tsapp/src/lib/auth/sso/__tests__/claim-mapping.test.tsapp/src/lib/auth/sso/__tests__/env-provider.test.tsapp/src/lib/auth/sso/__tests__/provider-cache.test.tsapp/src/lib/auth/sso/claim-mapping.tsapp/src/lib/auth/sso/env-provider.tsapp/src/lib/auth/sso/provider-cache.tsapp/src/lib/auth/sso/provider-loader.tsdocs/.astro/collections/docs.schema.jsondocs/.astro/content-modules.mjsdocs/.astro/content.d.tsdocs/.astro/data-store.jsondocs/.astro/settings.jsondocs/astro.config.mjsdocs/src/content/docs/administration/index.mdxdocs/src/content/docs/administration/managing-users.mdxdocs/src/content/docs/administration/multi-tenancy.mdxdocs/src/content/docs/authentication/api-keys.mdxdocs/src/content/docs/authentication/index.mdxdocs/src/content/docs/authentication/password-login.mdxdocs/src/content/docs/authentication/roles.mdxdocs/src/content/docs/authentication/sso.mdxdocs/src/content/docs/charts/param-select.mdxdocs/src/content/docs/concepts/index.mdxdocs/src/content/docs/connections/connecting-databases.mdxdocs/src/content/docs/connections/connectors.mdxdocs/src/content/docs/connections/index.mdxdocs/src/content/docs/connections/query-safety.mdxdocs/src/content/docs/dashboards/first-dashboard.mdxdocs/src/content/docs/dashboards/index.mdxdocs/src/content/docs/dashboards/overview.mdxdocs/src/content/docs/dashboards/parameters.mdxdocs/src/content/docs/dashboards/widgets.mdxdocs/src/content/docs/developer/architecture.mdxdocs/src/content/docs/developer/index.mdxdocs/src/content/docs/getting-started/configuration.mdxdocs/src/content/docs/getting-started/quick-start.mdxdocs/src/content/docs/guides/index.mdxdocs/src/content/docs/index.mdx
💤 Files with no reviewable changes (2)
- docs/src/content/docs/guides/index.mdx
- docs/src/content/docs/concepts/index.mdx
✅ Files skipped from review due to trivial changes (20)
- docs/src/content/docs/charts/param-select.mdx
- docs/src/content/docs/dashboards/widgets.mdx
- docs/src/content/docs/getting-started/quick-start.mdx
- docs/src/content/docs/administration/managing-users.mdx
- docs/src/content/docs/index.mdx
- docs/.astro/settings.json
- docs/src/content/docs/authentication/password-login.mdx
- docs/src/content/docs/connections/index.mdx
- docs/src/content/docs/developer/index.mdx
- docs/src/content/docs/dashboards/first-dashboard.mdx
- docs/src/content/docs/authentication/index.mdx
- docs/src/content/docs/connections/connecting-databases.mdx
- docs/src/content/docs/administration/index.mdx
- docs/src/content/docs/dashboards/index.mdx
- app/src/lib/auth/sso/claim-mapping.ts
- docs/src/content/docs/authentication/sso.mdx
- docs/src/content/docs/developer/architecture.mdx
- app/src/lib/auth/sso/tests/claim-mapping.test.ts
- docs/.astro/collections/docs.schema.json
- docs/.astro/content-modules.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/lib/auth/sso/provider-loader.ts
Comment on lines
+91
to
+96
| export function getLiveCollection<C extends keyof LiveContentConfig['collections']>( | ||
| collection: C, | ||
| filter?: LiveLoaderCollectionFilterType<C>, | ||
| ): Promise< | ||
| import('astro').LiveDataCollectionResult<LiveLoaderDataType<C>, LiveLoaderErrorType<C>> | ||
| >; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the astro version installed in the docs workspace.
# Expected: version >= 5.10.0 (experimental) or >= 6.0.0 (stable) to support LiveLoader APIs.
fd -g 'package.json' --search-path docs --max-depth 2 --exec grep -H '"astro"'Repository: alfredo1996/neoboard
Length of output: 104
🌐 Web query:
Astro LiveLoader API release notes when added 5.10 6.0
💡 Result:
The Astro LiveLoader API (part of Live Content Collections) was introduced experimentally in Astro 5.10.0 [1][2], requiring the experimental.liveContentCollections flag in astro.config.* [3][1]. It became stable in Astro 6.0.0 [3][4], where the flag must be removed, and collections are defined in src/live.config.ts using defineLiveCollection with a LiveLoader object [3][5][6]. The LiveLoader provides loadCollection and loadEntry methods for runtime data fetching, with explicit error handling [5][7][6]. Review patch releases from 5.10.2+ for breaking changes during the experimental phase [3]. Official docs detail the full API [6][5].
Citations:
- 1: https://astro.build/blog/astro-5100
- 2: feat: live content collections withastro/astro#13685
- 3: https://github.com/withastro/astro/releases/tag/astro@6.0.0
- 4: https://astro.build/blog/astro-6/
- 5: https://docs.astro.build/ar/reference/content-loader-reference/
- 6: https://docs.astro.build/en/reference/content-loader-reference
- 7: feat(live loaders): rename functions and add error handling withastro/astro#13846
Upgrade Astro to support Live Content Collections API
The generated types in docs/.astro/content.d.ts (lines 91–96, 134–137, 191–219) reference LiveLoader, LiveDataCollectionResult, and LiveDataEntryResult, which were introduced experimentally in Astro 5.10.0 and became stable in Astro 6.0.0. The declared version in docs/package.json is 5.3.0, which does not support this API.
Upgrade Astro in the docs workspace to at least 5.10.0 (for experimental support with flag) or 6.0.0+ (for stable support).
🤖 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/.astro/content.d.ts` around lines 91 - 96, The generated types in
docs/.astro/content.d.ts reference LiveLoader, LiveDataCollectionResult, and
LiveDataEntryResult (see getLiveCollection signature), but the docs workspace
declares Astro 5.3.0 which lacks that API; update the docs workspace Astro
dependency in docs/package.json to at least 6.0.0 (or 5.10.0+ if you intend to
enable experimental flags), run package install to update lockfiles, then
regenerate the docs types by running the docs build/dev command (so content.d.ts
is recreated against the upgraded Astro). Ensure the upgrade affects the
devDependencies used to produce docs/.astro/content.d.ts and verify the
getLiveCollection and related type signatures compile cleanly.
npm ci in CI was failing because package.json lists the enterprise workspace but package-lock.json didn't include it. Regenerated the lockfile to include the enterprise package dependency tree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/lib/auth/sso/env-provider.ts (1)
49-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
OIDC_DEFAULT_ROLEforce-cast still unvalidated.The unsafe
as UserRolecast is still present and unresolved — an invalid env value silently propagates into provisioning/role-sync at login time.🛡️ Proposed fix
+ const envDefaultRole = process.env.OIDC_DEFAULT_ROLE; + const defaultRole: UserRole = + envDefaultRole === "admin" || + envDefaultRole === "creator" || + envDefaultRole === "reader" + ? envDefaultRole + : "creator"; + return { ... metadata: { - defaultRole: (process.env.OIDC_DEFAULT_ROLE as UserRole) || "creator", + defaultRole,🤖 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/lib/auth/sso/env-provider.ts` at line 49, The OIDC_DEFAULT_ROLE value is being force-cast to UserRole without validation in the defaultRole assignment; replace the unsafe cast by validating the env value against the UserRole set (e.g., via a helper isValidUserRole(value) or a getDefaultRoleFromEnv function) and only use it if it matches a known UserRole, otherwise fall back to "creator"; update the assignment where defaultRole is set in env-provider.ts to call that validator/helper so invalid env values cannot silently propagate.
🧹 Nitpick comments (1)
app/src/lib/auth/sso/__tests__/env-provider.test.ts (1)
144-153: ⚡ Quick winAdd a test for an invalid
OIDC_DEFAULT_ROLEvalue.The suite only validates known-good roles. An invalid string (e.g.
"superadmin") will silently pass through the current source and be returned verbatim — a test for that path both documents the expected fallback ("creator") and forces the fix inenv-provider.ts.✅ Suggested test
+ it("falls back to 'creator' for invalid OIDC_DEFAULT_ROLE", async () => { + vi.stubEnv("NEOBOARD_EDITION", "enterprise"); + vi.stubEnv("OIDC_ISSUER", "https://idp.example.com"); + vi.stubEnv("OIDC_CLIENT_ID", "neoboard"); + vi.stubEnv("OIDC_CLIENT_SECRET", "secret123"); + vi.stubEnv("OIDC_DEFAULT_ROLE", "superadmin"); + const { loadEnvSsoProvider } = await import("../env-provider"); + const provider = loadEnvSsoProvider(); + expect(provider!.metadata.defaultRole).toBe("creator"); + });🤖 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/lib/auth/sso/__tests__/env-provider.test.ts` around lines 144 - 153, Add a test that stubs OIDC_DEFAULT_ROLE to an invalid value (e.g. "superadmin") and asserts loadEnvSsoProvider().metadata.defaultRole falls back to "creator"; this will require updating env-provider.ts's loadEnvSsoProvider (or the helper that parses OIDC_DEFAULT_ROLE) to validate the supplied role against the allowed set (e.g. "creator", "reader", etc.) and return "creator" when the env value is not one of the known roles. Locate the logic in loadEnvSsoProvider/parseDefaultRole and implement the whitelist check and fallback so the new test fails until fixed.
🤖 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/src/content/docs/authentication/roles.mdx`:
- Line 19: The doc currently contradicts itself about Admin write permissions:
update the "Run write queries" table row (the cell currently "Yes (if enabled)")
and the prose that describes Admin behavior (the paragraph covering Admin at
lines 39–43) so both reflect the actual implementation; choose one of two
fixes—either change both the table cell and the Admin paragraph to "Yes" (Admin
can write regardless of toggle) or change both to "Yes (if enabled)" (Admin
write requires the toggle)—and make sure the text/string "Run write queries",
the Admin role paragraph, and any surrounding policy wording are updated to
match exactly.
---
Duplicate comments:
In `@app/src/lib/auth/sso/env-provider.ts`:
- Line 49: The OIDC_DEFAULT_ROLE value is being force-cast to UserRole without
validation in the defaultRole assignment; replace the unsafe cast by validating
the env value against the UserRole set (e.g., via a helper
isValidUserRole(value) or a getDefaultRoleFromEnv function) and only use it if
it matches a known UserRole, otherwise fall back to "creator"; update the
assignment where defaultRole is set in env-provider.ts to call that
validator/helper so invalid env values cannot silently propagate.
---
Nitpick comments:
In `@app/src/lib/auth/sso/__tests__/env-provider.test.ts`:
- Around line 144-153: Add a test that stubs OIDC_DEFAULT_ROLE to an invalid
value (e.g. "superadmin") and asserts loadEnvSsoProvider().metadata.defaultRole
falls back to "creator"; this will require updating env-provider.ts's
loadEnvSsoProvider (or the helper that parses OIDC_DEFAULT_ROLE) to validate the
supplied role against the allowed set (e.g. "creator", "reader", etc.) and
return "creator" when the env value is not one of the known roles. Locate the
logic in loadEnvSsoProvider/parseDefaultRole and implement the whitelist check
and fallback so the new test fails until fixed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f8bedd0-e383-40bd-b3ca-eed361b34b20
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
.env.exampleapp/src/app/api/auth/sso-providers/route.tsapp/src/lib/auth/sso/__tests__/claim-mapping.test.tsapp/src/lib/auth/sso/__tests__/env-provider.test.tsapp/src/lib/auth/sso/__tests__/provider-cache.test.tsapp/src/lib/auth/sso/claim-mapping.tsapp/src/lib/auth/sso/env-provider.tsapp/src/lib/auth/sso/provider-cache.tsapp/src/lib/auth/sso/provider-loader.tsdocs/.astro/collections/docs.schema.jsondocs/.astro/content-modules.mjsdocs/.astro/content.d.tsdocs/.astro/data-store.jsondocs/.astro/settings.jsondocs/astro.config.mjsdocs/src/content/docs/administration/index.mdxdocs/src/content/docs/administration/managing-users.mdxdocs/src/content/docs/administration/multi-tenancy.mdxdocs/src/content/docs/authentication/api-keys.mdxdocs/src/content/docs/authentication/index.mdxdocs/src/content/docs/authentication/password-login.mdxdocs/src/content/docs/authentication/roles.mdxdocs/src/content/docs/authentication/sso.mdxdocs/src/content/docs/charts/param-select.mdxdocs/src/content/docs/concepts/index.mdxdocs/src/content/docs/connections/connecting-databases.mdxdocs/src/content/docs/connections/connectors.mdxdocs/src/content/docs/connections/index.mdxdocs/src/content/docs/connections/query-safety.mdxdocs/src/content/docs/dashboards/first-dashboard.mdxdocs/src/content/docs/dashboards/index.mdxdocs/src/content/docs/dashboards/overview.mdxdocs/src/content/docs/dashboards/parameters.mdxdocs/src/content/docs/dashboards/widgets.mdxdocs/src/content/docs/developer/architecture.mdxdocs/src/content/docs/developer/index.mdxdocs/src/content/docs/getting-started/configuration.mdxdocs/src/content/docs/getting-started/quick-start.mdxdocs/src/content/docs/guides/index.mdxdocs/src/content/docs/index.mdx
💤 Files with no reviewable changes (2)
- docs/src/content/docs/guides/index.mdx
- docs/src/content/docs/concepts/index.mdx
✅ Files skipped from review due to trivial changes (19)
- docs/src/content/docs/dashboards/index.mdx
- docs/src/content/docs/administration/index.mdx
- docs/.astro/settings.json
- docs/src/content/docs/getting-started/quick-start.mdx
- docs/src/content/docs/index.mdx
- docs/src/content/docs/charts/param-select.mdx
- docs/src/content/docs/dashboards/first-dashboard.mdx
- docs/src/content/docs/connections/connecting-databases.mdx
- docs/src/content/docs/authentication/index.mdx
- .env.example
- docs/src/content/docs/getting-started/configuration.mdx
- docs/src/content/docs/developer/architecture.mdx
- docs/src/content/docs/dashboards/widgets.mdx
- docs/.astro/collections/docs.schema.json
- docs/src/content/docs/developer/index.mdx
- docs/src/content/docs/administration/managing-users.mdx
- docs/src/content/docs/authentication/sso.mdx
- docs/src/content/docs/authentication/password-login.mdx
- docs/.astro/content-modules.mjs
🚧 Files skipped from review as they are similar to previous changes (9)
- app/src/lib/auth/sso/tests/claim-mapping.test.ts
- app/src/lib/auth/sso/tests/provider-cache.test.ts
- docs/astro.config.mjs
- app/src/lib/auth/sso/claim-mapping.ts
- app/src/app/api/auth/sso-providers/route.ts
- docs/src/content/docs/connections/index.mdx
- app/src/lib/auth/sso/provider-loader.ts
- app/src/lib/auth/sso/provider-cache.ts
- docs/.astro/content.d.ts
| | Manage users | Yes | No | No | | ||
| | Access settings | Yes | No | No | | ||
| | Run read queries | Yes | Yes | No | | ||
| | Run write queries | Yes (if enabled) | Yes (if enabled) | No | |
There was a problem hiding this comment.
Resolve conflicting Admin write-permission behavior.
Line 19 says Admin can run write queries only if enabled, but Lines 39–43 say Admin can write regardless of toggle. Please make these statements consistent with actual product behavior to avoid policy confusion.
Suggested doc fix (pick the variant that matches implementation)
-| Run write queries | Yes (if enabled) | Yes (if enabled) | No |
+| Run write queries | Yes (always) | Yes (if enabled) | No |or
-| Admin | Can write | Can write (always) |
+| Admin | Can write | Read-only queries |
...
-Admins always have write access regardless of the toggle.
+Admins follow the write permission toggle like Creators.Also applies to: 39-43
🤖 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/src/content/docs/authentication/roles.mdx` at line 19, The doc currently
contradicts itself about Admin write permissions: update the "Run write queries"
table row (the cell currently "Yes (if enabled)") and the prose that describes
Admin behavior (the paragraph covering Admin at lines 39–43) so both reflect the
actual implementation; choose one of two fixes—either change both the table cell
and the Admin paragraph to "Yes" (Admin can write regardless of toggle) or
change both to "Yes (if enabled)" (Admin write requires the toggle)—and make
sure the text/string "Run write queries", the Admin role paragraph, and any
surrounding policy wording are updated to match exactly.
With SSO enabled, the login page has both "Sign in with [Provider]" and
"Sign in" buttons. Tests using getByRole('button', { name: 'Sign in' })
fail with strict mode violation. Add exact: true to all instances.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The wildcard CORS header on /api/openapi.json was removed in #689. The E2E test still expected `access-control-allow-origin: *`. Updated to assert the header is undefined, matching the unit test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Multi-tenancy violations: - Add tenantId filter to credentials email lookup in auth config - Add tenantId filter to JWT callback DB re-fetch - Add tenantId to SSO user provision/update predicates - Add tenantId to signIn callback user existence check and update Race conditions: - SSO provider creation now relies on DB unique constraint for duplicate detection instead of non-atomic read-then-write checks - SSO user provisioning uses ON CONFLICT DO UPDATE for safe concurrent logins Security hardening: - Warn when TENANT_ID is not set (defaults to "default" for single-tenant) - Return false when SSO provider config not found in signIn callback - Validate OIDC_DEFAULT_ROLE against allowed values - Distinguish module-not-found from runtime errors in enterprise extension - Set explicit least-privilege permissions in CI workflow - Handle 204/non-JSON responses in fetchJson hook - Don't cache fallback results from DB errors in provider cache Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
7 tasks
alfredo1996
added a commit
that referenced
this pull request
May 10, 2026
* feat(enterprise): SSO providers schema, API, OIDC auth flow (M1+M2) Add enterprise SSO infrastructure: M1 — Data Model + API: - sso_providers Drizzle schema with encrypted client secrets (AES-256-GCM) - Admin-only CRUD API with tenant isolation and max 5 providers limit - Public listing endpoint for login page SSO buttons - Core extension point system (bootstrapExtensions) that dynamically loads @neoboard/enterprise when NEOBOARD_EDITION=enterprise - Enterprise package as npm workspace with sync-public workflow M2 — OIDC Auth Flow: - Claim-based role mapping (IdP claims → NeoBoard roles, with dot-notation support for nested claims like realm_access.roles) - User provisioning/linking: auto-provision toggle, account linking by email+tenantId, role sync on every login - Provider loader: converts DB rows to Auth.js OIDC provider configs with decrypted secrets Closes #693, closes #694, closes #695, closes #696, closes #697 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(enterprise): Settings > Authentication admin page (M3) Add admin UI for managing SSO providers: - New Authentication tab in Settings (Shield icon) - Provider list with name, issuer, status badge, default role, SSO enforcement - Add Provider dialog with OIDC config fields, claim mapping UI (IdP claim key → admin/creator/reader values), provisioning toggles, default role - Delete with confirmation (keeps users, disables SSO login) - useSsoProviders TanStack Query hook for CRUD operations - 6 component tests (loading, empty state, provider list, add dialog, delete confirmation, disabled badge) Closes #698 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(a11y,e2e): add missing DialogDescription, fix E2E login flake Three fixes: 1. Add sr-only DialogDescription to 9 dialogs that were missing it, silencing the Radix "Missing Description" console warning. Two multi-step dialogs use aria-describedby={undefined} to opt out. Closes #704 2. Fix E2E login flake (95 failures → 0) with two changes: - Pre-warm /login and auth API routes in global-setup so webpack compiles them before any test runs (eliminates 3-6s cold start) - Wait for React 19 hydration (form __reactFiber) in AuthPage.login before interacting with the form, with 5 retries and 15s timeout 3. Fix breadcrumb Storybook story: add asChild to DropdownMenuTrigger to avoid nested button HTML. Updates #703 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(enterprise): Auth.js dynamic OIDC wiring + login page SSO buttons (M4) Wire SSO providers into Auth.js and add SSO buttons to the login page: - Refactor NextAuth to lazy config (async function) so OIDC providers are loaded from the DB on each auth flow with 60s in-memory cache - Add signIn callback: detect SSO login, resolve role from IdP claims, provision/link user, populate JWT with SSO user data - Login page: fetch enabled SSO providers, render "Sign in with [Name]" buttons above password form with divider - SSO enforcement: when enforceSso is set, hide password form; admins can bypass via ?password=true query param - Public endpoint returns enforceSso flag in response meta - Provider cache with TTL (6 tests), config test updated for lazy init Closes #699, closes #700 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(enterprise): E2E SSO flow with Keycloak Testcontainer (M5) Full OIDC E2E tests against a real Keycloak IdP: - Keycloak container added to global-setup (start alongside PG/Neo4j) - Realm JSON with test users in groups (admins, editors, viewers) - SSO provider seeded in sso_providers table with encrypted secret - 5 E2E test scenarios: SSO button visible, redirect to Keycloak, full login with auto-provision, admin role via claim mapping, provider visible in Settings > Authentication Also fixes: - signIn callback simplified: let DrizzleAdapter handle user creation, signIn only checks auto-provision and syncs role from claims - allowDangerousEmailAccountLinking on SSO providers for account linking - callbackUrl: "/" on SSO signIn to redirect to dashboard after login - AuthPage.login uses exact match for "Sign in" button (avoids matching "Sign in with [Provider]" SSO buttons) Closes #701 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add SSO configuration guide with IdP examples Add comprehensive SSO documentation: - New guides/sso.mdx with setup instructions, role mapping, account linking, troubleshooting, and provider deletion behavior - Full configuration examples for Okta, Azure AD/Entra, Keycloak, and Google Workspace - Add Core/Enterprise badges to all guide pages via Starlight frontmatter badges - Fix enterprise variables in configuration.mdx (was SSO_ENABLED, now correctly references NEOBOARD_EDITION=enterprise) - Add SSO link to guides index page Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(auth): env-based SSO config — single OIDC provider via env vars Add env var support for SSO as the primary config method (Option C): - New env-provider.ts: reads OIDC_* env vars, returns LoadedSsoProvider when OIDC_ISSUER + OIDC_CLIENT_ID + OIDC_CLIENT_SECRET are all set - Works in community edition — no NEOBOARD_EDITION=enterprise needed - Env provider merges with DB providers (env first, DB adds on top) - Public /api/auth/sso-providers endpoint includes env provider - 14 unit tests for env-provider, 3 new cache merge tests - Docs updated: env vars as primary method, Docker Compose example, Admin UI moved to "Multi-Provider" subsection - Configuration docs: OIDC vars in the variables table Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(auth): support comma-separated values in SSO role mapping Allow multiple IdP groups to map to the same NeoBoard role using comma-separated values: OIDC_ADMIN_VALUE=devops,platform-team,it-ops OIDC_CREATOR_VALUE=engineering,data-team Each mapping value is split by comma and trimmed. If any of the user's claim values matches any target, the role is assigned. Priority order (admin > creator > reader) preserved. Backwards compatible — single values without commas work as before. 6 new tests added (19 total for claim mapping). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: document comma-separated values in SSO role mapping Update SSO guide to show multi-group role mapping: - Variable table notes comma-separated support - Role Mapping section with env var examples - Docker Compose example uses comma-separated groups Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(auth): gate env-based SSO behind NEOBOARD_EDITION=enterprise SSO (both env vars and Admin UI) now requires NEOBOARD_EDITION=enterprise. loadEnvSsoProvider() returns null unless the enterprise edition is active. Updated all tests and docs to reflect the requirement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: fix SSO docs to consistently require NEOBOARD_EDITION=enterprise - .env.example: remove "works in community edition", add NEOBOARD_EDITION=enterprise to required vars - configuration.mdx: note enterprise requirement in SSO section - sso.mdx: add NEOBOARD_EDITION to prerequisites list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: restructure documentation into topic-based sections (#705, #708) Major docs reorganization — from 6 generic sections to 8 topic-focused sections: Structure changes: - NEW Authentication/ — Password Login, SSO, API Keys, Roles & Permissions - NEW Dashboards/ — First Dashboard, Widgets, Parameters, Overview - NEW Connections/ — Connecting Databases, Connectors, Query Safety - NEW Administration/ — Managing Users, Multi-tenancy - REMOVED Concepts/ — merged into relevant sections - REMOVED User Guides/ — split across Authentication, Dashboards, Connections - KEPT Getting Started/, Chart Types/, CLI/, Developer Guide/ New pages: - authentication/password-login.mdx — session management, rate limiting, password requirements, forced password change, registration control - authentication/roles.mdx — role comparison table, write permissions, SSO role assignment Developer docs (#708): - Mermaid architecture diagram (packages + dependencies) - "Where Does Code Live?" table (feature → package → directory) - "Common Tasks" section (add API route, chart type, setting, DB table) - Newcomer-friendly numbered getting-started flow on index page All cross-references updated to new paths. Closes #705, closes #708 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ci): update package-lock.json with enterprise workspace npm ci in CI was failing because package.json lists the enterprise workspace but package-lock.json didn't include it. Regenerated the lockfile to include the enterprise package dependency tree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(e2e): use exact match for 'Sign in' button in all E2E tests With SSO enabled, the login page has both "Sign in with [Provider]" and "Sign in" buttons. Tests using getByRole('button', { name: 'Sign in' }) fail with strict mode violation. Add exact: true to all instances. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(e2e): update CORS test to match #689 security hardening The wildcard CORS header on /api/openapi.json was removed in #689. The E2E test still expected `access-control-allow-origin: *`. Updated to assert the header is undefined, matching the unit test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(auth,security): address CodeRabbit findings on PR #702 Multi-tenancy violations: - Add tenantId filter to credentials email lookup in auth config - Add tenantId filter to JWT callback DB re-fetch - Add tenantId to SSO user provision/update predicates - Add tenantId to signIn callback user existence check and update Race conditions: - SSO provider creation now relies on DB unique constraint for duplicate detection instead of non-atomic read-then-write checks - SSO user provisioning uses ON CONFLICT DO UPDATE for safe concurrent logins Security hardening: - Warn when TENANT_ID is not set (defaults to "default" for single-tenant) - Return false when SSO provider config not found in signIn callback - Validate OIDC_DEFAULT_ROLE against allowed values - Distinguish module-not-found from runtime errors in enterprise extension - Set explicit least-privilege permissions in CI workflow - Handle 204/non-JSON responses in fetchJson hook - Don't cache fallback results from DB errors in provider cache Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
12 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
bootstrapExtensions())sso_providerstable — Drizzle schema + migration with encrypted client secrets (AES-256-GCM), claim mappings (JSONB), tenant isolation/api/sso-providers) — GET/POST/DELETE withrequireAdmin(), max 5 providers per tenant, secrets never exposed in responses/api/auth/sso-providers) — returns only id + name of enabled providers for login page SSO buttonsrealm_access.roles), priority: admin > creator > readercanWritefollows role defaultssync-public.ymlworkflow to stripenterprise/for public mirrorTest plan
npm run build)Closes #693, #694, #695, #696, #697
🤖 Generated with Claude Code
Summary by CodeRabbit