From bdefb340590a686ae0ddd2e94819bf77db46b719 Mon Sep 17 00:00:00 2001 From: "groombook-engineer[bot]" <269742240+groombook-engineer[bot]@users.noreply.github.com> Date: Fri, 3 Apr 2026 07:17:12 +0000 Subject: [PATCH] fix(api): needsSetup guard ordering in setup auth endpoints (GRO-392 UAT fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(oobe): add conditional auth provider bootstrap step (GRO-392) Backend: - GET /api/setup/status now returns showAuthProviderStep, authConfigExists, and authEnvVarsSet to inform the frontend whether to show the step - POST /api/setup/auth-provider: unauthenticated endpoint for first-time auth provider configuration during OOBE; guarded by needsSetup check (returns 403 after setup completes); encrypts clientSecret before storing Frontend: - SetupWizard fetches /api/setup/status on mount to determine if the auth provider step is needed (fresh install with no DB config and no OIDC env vars) - When needed, inserts the Auth Provider step after Welcome, before Business Name; includes full form with Test Connection button - Endpoint is POST /api/admin/auth-provider/test for connection testing Co-Authored-By: Paperclip * fix(oobe): add test connection endpoint and fix EOF newline (GRO-392) - Add POST /api/setup/auth-provider/test endpoint for OOBE test connection - Guard with same !superUser check as bootstrap endpoint - Update SetupWizard to call /api/setup/auth-provider/test instead of /api/admin/auth-provider/test (which requires auth session) - Add trailing newline at EOF in setup.ts Co-Authored-By: Paperclip * fix(oobe): remove unused catch variable in setup.ts (GRO-392) Co-Authored-By: Paperclip * feat(api): auth provider CRUD endpoints + test-connection (GRO-388) Implement admin API endpoints for managing auth provider configuration: - GET /api/admin/auth-provider — get current config (secret redacted) - PUT /api/admin/auth-provider — create or update provider config - POST /api/admin/auth-provider/test — validate via OIDC discovery endpoint - DELETE /api/admin/auth-provider — remove DB config (falls back to env vars) All endpoints are gated by requireSuperUser(). The clientSecret is AES-256-GCM encrypted before DB write and always redacted on return. Test-connection fetches /.well-known/openid-configuration and returns metadata on success or error detail on failure. Includes 16 unit tests covering all endpoints and error paths. Co-Authored-By: Paperclip * fix(api): requireRoleOrSuperUser for /admin/* routes (GRO-412) Fix bug where super users granted via Staff UI were blocked from admin routes because requireRole("manager") checked role before isSuperUser. Changed to requireRoleOrSuperUser("manager") so super users bypass the manager-role check. Also adds 7 unit tests for requireRoleOrSuperUser middleware covering: manager access, super user bypass, non-super-user blocking, and multi-role scenarios. Co-Authored-By: Paperclip * fix(api): remove unused decryptSecret import and eslint-disable directives Fixes lint error exposed by merge with main (GRO-392 PR #214) Co-Authored-By: Paperclip * fix(tests): use main's authProvider tests after rebase conflict resolution The rebase introduced incompatible test code from the pre-merge GRO-388 commit. Replaced with the canonical test file from main to ensure tests pass and reflect the actual router implementation. Co-Authored-By: Paperclip * fix(api): remove duplicate authProviderRouter import and route registration Rebase introduced duplicate import from ./routes/admin/authProvider.js and duplicate route registration. Removed duplicates since the correct import is from ./routes/authProvider.js. Co-Authored-By: Paperclip * fix(e2e): use lean schema for OIDC test endpoint; add trailing newline Fix CTO review comments on GRO-392: - POST /api/setup/auth-provider/test now uses authProviderTestSchema (only issuerUrl + internalBaseUrl) instead of full authProviderBootstrapSchema — clientSecret is not needed for OIDC discovery and was not being sent by the frontend handler - POST /api/admin/auth-provider/test already uses omit() correctly; no change needed - apps/api/src/routes/admin/authProvider.ts: added trailing newline Co-Authored-By: Paperclip * feat(web): add auth provider section to settings page (GRO-391) Add Authentication Provider section to /admin/settings for super users. Implements: provider ID, display name, issuer URL, internal base URL (optional, collapsed), client ID, client secret (masked, only sent on change), scopes fields; Test Connection button; Save and Reset to Environment Defaults with confirmation dialog; warning banner about service restart; env config info banner when no DB config is set. Co-Authored-By: Claude Opus 4.6 * fix(api): move needsSetup guard before Zod parsing in setup endpoints POST /api/setup/auth-provider and POST /api/setup/auth-provider/test were returning 400 (Zod validation) instead of 403 when needsSetup was false, because zValidator middleware ran before the route handler body. Now manually parse the body after the needsSetup guard so 403 fires immediately for post-setup requests. Co-Authored-By: Paperclip * fix(api): replace c.req.valid("json") with await c.req.json() Replace zValidator-orphaned c.req.valid("json") calls with await c.req.json() in the auth provider bootstrap and test endpoints per CTO review. Co-Authored-By: Paperclip --------- Co-authored-by: groombook-engineer[bot] <3141748+groombook-engineer[bot]@users.noreply.github.com> Co-authored-by: Paperclip Co-authored-by: Barkley Trimsworth Co-authored-by: Claude Opus 4.6 --- apps/api/src/routes/admin/authProvider.ts | 2 +- apps/api/src/routes/setup.ts | 14 +- apps/web/src/pages/Settings.tsx | 436 ++++++++++++++++++++++ 3 files changed, 447 insertions(+), 5 deletions(-) diff --git a/apps/api/src/routes/admin/authProvider.ts b/apps/api/src/routes/admin/authProvider.ts index fa7b79c..e8acd15 100644 --- a/apps/api/src/routes/admin/authProvider.ts +++ b/apps/api/src/routes/admin/authProvider.ts @@ -187,4 +187,4 @@ authProviderRouter.delete("/", requireSuperUser(), async (c) => { await db.delete(authProviderConfig).where(eq(authProviderConfig.id, existing.id)); return c.json({ ok: true, message: "Auth provider config removed; auth will fall back to env vars" }); -}); \ No newline at end of file +}); diff --git a/apps/api/src/routes/setup.ts b/apps/api/src/routes/setup.ts index 87d0fcb..37bb3a2 100644 --- a/apps/api/src/routes/setup.ts +++ b/apps/api/src/routes/setup.ts @@ -110,6 +110,12 @@ const authProviderBootstrapSchema = z.object({ scopes: z.string().default("openid profile email"), }); +// Minimal schema for test endpoint — OIDC discovery only needs issuer/internal URLs +const authProviderTestSchema = z.object({ + issuerUrl: z.string().url(), + internalBaseUrl: z.string().url().nullable().optional(), +}); + /** * POST /api/setup/auth-provider * Unauthenticated endpoint for first-time auth provider setup during OOBE. @@ -117,7 +123,7 @@ const authProviderBootstrapSchema = z.object({ * Rate-limited by the API gateway; additionally restricted to first-time setup only. * After setup completes, this endpoint permanently returns 403. */ -setupRouter.post("/auth-provider", zValidator("json", authProviderBootstrapSchema), async (c) => { +setupRouter.post("/auth-provider", async (c) => { const db = getDb(); // Guard: only allow during fresh install (no super user yet) @@ -143,7 +149,7 @@ setupRouter.post("/auth-provider", zValidator("json", authProviderBootstrapSchem return c.json({ error: "Auth provider is already configured." }, 409); } - const body = c.req.valid("json"); + const body = authProviderBootstrapSchema.parse(await c.req.json()); // Encrypt clientSecret before storing const encryptedSecret = encryptSecret(body.clientSecret); @@ -186,7 +192,7 @@ setupRouter.post("/auth-provider", zValidator("json", authProviderBootstrapSchem * Fetches the OIDC discovery document to confirm the issuer is reachable. * Only available when needsSetup is true (no super user = fresh install). */ -setupRouter.post("/auth-provider/test", zValidator("json", authProviderBootstrapSchema), async (c) => { +setupRouter.post("/auth-provider/test", async (c) => { const db = getDb(); // Guard: only allow during fresh install (no super user yet) @@ -200,7 +206,7 @@ setupRouter.post("/auth-provider/test", zValidator("json", authProviderBootstrap return c.json({ ok: false, error: "Setup has already been completed." }, 403); } - const body = c.req.valid("json"); + const body = authProviderTestSchema.parse(await c.req.json()); // Determine the discovery URL const discoveryUrl = body.internalBaseUrl diff --git a/apps/web/src/pages/Settings.tsx b/apps/web/src/pages/Settings.tsx index 265d3e1..16b8ff2 100644 --- a/apps/web/src/pages/Settings.tsx +++ b/apps/web/src/pages/Settings.tsx @@ -1,6 +1,40 @@ import { useState, useEffect, useRef } from "react"; import { useBranding } from "../BrandingContext.js"; +interface AuthProviderConfig { + id: number; + providerId: string; + displayName: string; + issuerUrl: string; + internalBaseUrl: string | null; + clientId: string; + clientSecret: string; + scopes: string; + enabled: boolean; + createdAt: string; + updatedAt: string; +} + +interface AuthProviderForm { + providerId: string; + displayName: string; + issuerUrl: string; + internalBaseUrl: string; + clientId: string; + clientSecret: string; + scopes: string; +} + +const REDACTED = "••••••••"; + +interface CurrentUser { + id: string; + name: string; + email: string; + role: string; + isSuperUser: boolean; +} + interface SettingsForm { businessName: string; primaryColor: string; @@ -13,6 +47,28 @@ interface SettingsForm { export function SettingsPage() { const { refresh } = useBranding(); + const [currentUser, setCurrentUser] = useState(null); + + // Auth provider state + const [authConfig, setAuthConfig] = useState(null); + const [authForm, setAuthForm] = useState({ + providerId: "authentik", + displayName: "", + issuerUrl: "", + internalBaseUrl: "", + clientId: "", + clientSecret: "", + scopes: "openid profile email", + }); + const [authSecretTouched, setAuthSecretTouched] = useState(false); + const [authLoaded, setAuthLoaded] = useState(false); + const [authSaving, setAuthSaving] = useState(false); + const [authMessage, setAuthMessage] = useState<{ type: "success" | "error"; text: string } | null>(null); + const [testingConnection, setTestingConnection] = useState(false); + const [testResult, setTestResult] = useState<{ ok: boolean; error?: string } | null>(null); + const [showInternalBaseUrl, setShowInternalBaseUrl] = useState(false); + const [confirmReset, setConfirmReset] = useState(false); + const [form, setForm] = useState({ businessName: "", primaryColor: "#4f8a6f", @@ -57,6 +113,33 @@ export function SettingsPage() { .catch(() => setLoaded(true)); }, []); + // Load current user (for isSuperUser check) and auth provider config + useEffect(() => { + Promise.all([ + fetch("/api/staff/me").then((r) => r.json()).catch(() => null), + fetch("/api/admin/auth-provider").then(async (r) => { + if (r.ok) return r.json(); + if (r.status === 404) return null; + throw new Error(`HTTP ${r.status}`); + }).catch(() => null), + ]).then(([user, auth]) => { + setCurrentUser(user as CurrentUser | null); + if (auth) { + setAuthConfig(auth as AuthProviderConfig); + setAuthForm({ + providerId: (auth as AuthProviderConfig).providerId, + displayName: (auth as AuthProviderConfig).displayName, + issuerUrl: (auth as AuthProviderConfig).issuerUrl, + internalBaseUrl: (auth as AuthProviderConfig).internalBaseUrl ?? "", + clientId: (auth as AuthProviderConfig).clientId, + clientSecret: (auth as AuthProviderConfig).clientSecret, + scopes: (auth as AuthProviderConfig).scopes, + }); + } + setAuthLoaded(true); + }); + }, []); + const handleLogoChange = async (e: React.ChangeEvent) => { const file = e.target.files?.[0]; if (!file) return; @@ -143,6 +226,105 @@ export function SettingsPage() { } }; + // Auth provider handlers + const handleTestConnection = async () => { + setTestingConnection(true); + setTestResult(null); + try { + const res = await fetch("/api/admin/auth-provider/test", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + providerId: authForm.providerId, + issuerUrl: authForm.issuerUrl, + clientId: authForm.clientId, + }), + }); + const data = await res.json(); + setTestResult(data); + } catch { + setTestResult({ ok: false, error: "Network error. Please try again." }); + } finally { + setTestingConnection(false); + } + }; + + const handleAuthSave = async () => { + setAuthSaving(true); + setAuthMessage(null); + try { + const payload: Record = { + providerId: authForm.providerId, + displayName: authForm.displayName, + issuerUrl: authForm.issuerUrl, + clientId: authForm.clientId, + scopes: authForm.scopes, + }; + if (authForm.internalBaseUrl) { + payload.internalBaseUrl = authForm.internalBaseUrl; + } + // Only send clientSecret if user changed it from the redacted value + if (authSecretTouched) { + payload.clientSecret = authForm.clientSecret; + } + const res = await fetch("/api/admin/auth-provider", { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(payload), + }); + if (!res.ok) { + const err = await res.json().catch(() => null); + throw new Error(err?.error ?? "Failed to save auth provider"); + } + const saved = await res.json() as AuthProviderConfig; + setAuthConfig(saved); + setAuthForm({ + providerId: saved.providerId, + displayName: saved.displayName, + issuerUrl: saved.issuerUrl, + internalBaseUrl: saved.internalBaseUrl ?? "", + clientId: saved.clientId, + clientSecret: saved.clientSecret, + scopes: saved.scopes, + }); + setAuthSecretTouched(false); + setAuthMessage({ type: "success", text: "Auth provider saved." }); + } catch (err: unknown) { + setAuthMessage({ type: "error", text: err instanceof Error ? err.message : "Save failed" }); + } finally { + setAuthSaving(false); + } + }; + + const handleResetToEnvDefaults = async () => { + if (!confirmReset) { + setConfirmReset(true); + return; + } + setConfirmReset(false); + try { + const res = await fetch("/api/admin/auth-provider", { method: "DELETE" }); + if (!res.ok) { + const err = await res.json().catch(() => null); + throw new Error(err?.error ?? "Failed to reset auth provider"); + } + setAuthConfig(null); + setAuthForm({ + providerId: "authentik", + displayName: "", + issuerUrl: "", + internalBaseUrl: "", + clientId: "", + clientSecret: "", + scopes: "openid profile email", + }); + setAuthSecretTouched(false); + setAuthMessage({ type: "success", text: "Auth provider reset to environment defaults." }); + } catch (err: unknown) { + setAuthMessage({ type: "error", text: err instanceof Error ? err.message : "Reset failed" }); + } + }; + if (!loaded) return

Loading settings...

; const logoSrc = form.logoUrl ?? (form.logoBase64 && form.logoMimeType ? `data:${form.logoMimeType};base64,${form.logoBase64}` : null); @@ -385,6 +567,260 @@ export function SettingsPage() { > {saving ? "Saving..." : "Save Changes"} + + {/* Auth Provider Section — super users only */} + {currentUser?.isSuperUser && ( + <> +
+

Authentication Provider

+

+ Configure the SSO provider for sign-in. Changes require a service restart. +

+ + {/* Warning banner */} +
+ ⚠️ Changing auth settings will require a service restart. Active sessions will be preserved. +
+ + {/* Environment config banner */} + {!authConfig && authLoaded && ( +
+ Currently using environment configuration (no DB config set). +
+ )} + + {!authLoaded &&

Loading auth provider...

} + + {authLoaded && ( + <> +
+ {/* Provider ID */} +
+ + setAuthForm((f) => ({ ...f, providerId: e.target.value }))} + placeholder="e.g. authentik, okta" + style={{ width: "100%", padding: "0.5rem 0.75rem", border: "1px solid #d1d5db", borderRadius: 6, fontSize: 14 }} + /> +
+ + {/* Display Name */} +
+ + setAuthForm((f) => ({ ...f, displayName: e.target.value }))} + placeholder="e.g. Company SSO" + style={{ width: "100%", padding: "0.5rem 0.75rem", border: "1px solid #d1d5db", borderRadius: 6, fontSize: 14 }} + /> +
+ + {/* Issuer URL */} +
+ +
+ setAuthForm((f) => ({ ...f, issuerUrl: e.target.value }))} + placeholder="https://your-idp.example.com" + style={{ flex: 1, padding: "0.5rem 0.75rem", border: "1px solid #d1d5db", borderRadius: 6, fontSize: 14 }} + /> + +
+
+ + {/* Test result */} + {testResult && ( +
+ {testResult.ok ? "✓ Connection successful" : `✗ ${testResult.error}`} +
+ )} + + {/* Internal Base URL — collapsible */} +
+ + {showInternalBaseUrl && ( + setAuthForm((f) => ({ ...f, internalBaseUrl: e.target.value }))} + placeholder="http://host.docker.internal:9080" + style={{ marginTop: 4, width: "100%", padding: "0.5rem 0.75rem", border: "1px solid #d1d5db", borderRadius: 6, fontSize: 14 }} + /> + )} +
+ + {/* Client ID */} +
+ + setAuthForm((f) => ({ ...f, clientId: e.target.value }))} + style={{ width: "100%", padding: "0.5rem 0.75rem", border: "1px solid #d1d5db", borderRadius: 6, fontSize: 14 }} + /> +
+ + {/* Client Secret */} +
+ + { + setAuthSecretTouched(true); + setAuthForm((f) => ({ ...f, clientSecret: e.target.value })); + }} + placeholder={authConfig ? "(unchanged)" : "Required"} + style={{ width: "100%", padding: "0.5rem 0.75rem", border: "1px solid #d1d5db", borderRadius: 6, fontSize: 14 }} + /> + {authConfig && !authSecretTouched && ( +

Leave blank to keep existing secret.

+ )} +
+ + {/* Scopes */} +
+ + setAuthForm((f) => ({ ...f, scopes: e.target.value }))} + style={{ width: "100%", padding: "0.5rem 0.75rem", border: "1px solid #d1d5db", borderRadius: 6, fontSize: 14 }} + /> +
+
+ + {/* Auth messages */} + {authMessage && ( +
+ {authMessage.text} +
+ )} + + {/* Action buttons */} +
+ + + {confirmReset && ( + + )} +
+ + )} + + )} ); }