diff --git a/apps/admin/src/App.tsx b/apps/admin/src/App.tsx index 8f8d834f..86bcc321 100644 --- a/apps/admin/src/App.tsx +++ b/apps/admin/src/App.tsx @@ -47,6 +47,7 @@ import OrgBillingPage from './pages/organization/org-billing'; import OrgInvoicesPage from './pages/organization/org-invoices'; import OrgInvoiceDetailPage from './pages/organization/org-invoice-detail'; import OrgLegalDetailsPage from './pages/organization/org-legal-details'; +import OrgSsoPage from './pages/organization/org-sso'; import OrgIntelligencePage from './pages/organization/org-intelligence'; import OrgObservabilityPage from './pages/organization/org-observability'; import AcceptInvitationPage from './pages/invitations/accept'; @@ -308,6 +309,14 @@ function App() { } /> + + + + } + /> ; +} + +function OrgSsoForm() { + const { t } = useTranslation(); + const { config, isLoading, error, updateConfig } = useSsoConfig(); + + const [formValues, setFormValues] = useState(DEFAULT_FORM_VALUES); + const [clientSecretInput, setClientSecretInput] = useState(''); + const [isSaving, setIsSaving] = useState(false); + const [submitError, setSubmitError] = useState(null); + + // useSsoConfig() rebuilds `config` via `stripClientSecret`'s object + // spread on every call (use-sso-config.ts), so it's a new object + // identity on every render even when the underlying data hasn't + // changed. A plain `[config]` dependency would therefore re-fire this + // effect - and stomp any in-progress edit back to the loaded values - + // on every keystroke, since each keystroke's setFormValues triggers a + // re-render that calls the hook again. Guard with a ref so the form is + // hydrated from the loaded config exactly once per mount, not once per + // render. + const hasHydratedRef = useRef(false); + + useEffect(() => { + if (!config || hasHydratedRef.current) { + return; + } + hasHydratedRef.current = true; + setFormValues({ + issuerUrl: config.issuerUrl ?? '', + clientId: config.clientId ?? '', + allowedDomains: (config.allowedDomains ?? []).join(', '), + enforceSso: config.enforceSso ?? false, + }); + }, [config]); + + async function handleSubmit(event: FormEvent) { + event.preventDefault(); + setSubmitError(null); + setIsSaving(true); + try { + await updateConfig({ + issuerUrl: formValues.issuerUrl, + clientId: formValues.clientId, + allowedDomains: formValues.allowedDomains + .split(',') + .map((domain) => domain.trim()) + .filter(Boolean), + enforceSso: formValues.enforceSso, + // Omit clientSecret entirely when the user didn't type a new one - + // never coerce to an empty string, which would defeat the + // optional-field contract on the backend. + ...(clientSecretInput ? { clientSecret: clientSecretInput } : {}), + }); + setClientSecretInput(''); + } catch { + setSubmitError(t('errors.failedToSaveConfiguration')); + } finally { + setIsSaving(false); + } + } + + return ( +
+
+

+

+

{t('sso.description')}

+
+ + {isLoading ? ( +
{t('common.loading')}
+ ) : ( +
+ {error && ( +
+
+ )} + +
+ + + setFormValues((prev) => ({ ...prev, issuerUrl: event.target.value })) + } + className={INPUT_CLASSES} + /> +
+ +
+ + + setFormValues((prev) => ({ ...prev, clientId: event.target.value })) + } + className={INPUT_CLASSES} + /> +
+ +
+ + {/* Never populate value/defaultValue with a real secret - only the + boolean hasClientSecret drives the "currently set" indicator. */} + setClientSecretInput(event.target.value)} + className={INPUT_CLASSES} + /> +

+ {t('sso.settings.clientSecretPlaceholder')} +

+
+ +
+ + + setFormValues((prev) => ({ ...prev, allowedDomains: event.target.value })) + } + className={INPUT_CLASSES} + /> +
+ +
+ + setFormValues((prev) => ({ ...prev, enforceSso: event.target.checked })) + } + className="mt-0.5 h-4 w-4 rounded border-gray-300 text-blue-600 focus:ring-blue-500" + /> + +
+ + {submitError && ( +
+
+ )} + + +
+ )} +
+ ); +} diff --git a/apps/admin/src/tests/pages/org-sso.test.tsx b/apps/admin/src/tests/pages/org-sso.test.tsx new file mode 100644 index 00000000..59275905 --- /dev/null +++ b/apps/admin/src/tests/pages/org-sso.test.tsx @@ -0,0 +1,220 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { userEvent } from '@testing-library/user-event'; +import OrgSsoPage from '../../pages/organization/org-sso'; +import { usePermissions } from '../../hooks/use-permissions'; +import { useSsoConfig } from '../../hooks/use-sso-config'; + +vi.mock('../../hooks/use-permissions', () => ({ + usePermissions: vi.fn(), +})); + +// useSsoConfig is a @tanstack/react-query hook; mocking the module +// directly avoids needing a QueryClientProvider ancestor, same fix as +// dedup-rules-back-nav.test.tsx uses for useDedupRules(). +vi.mock('../../hooks/use-sso-config', () => ({ + useSsoConfig: vi.fn(), +})); + +describe('OrgSsoPage', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('renders the SSO config form for an org admin', () => { + vi.mocked(usePermissions).mockReturnValue({ + isSystemAdmin: false, + orgRole: 'admin', + isLoading: false, + } as ReturnType); + vi.mocked(useSsoConfig).mockReturnValue({ + config: undefined, + isLoading: false, + error: null, + updateConfig: vi.fn(), + isSaving: false, + } as ReturnType); + + render(); + + // useSsoConfig() (#407 / PR #419) takes no arguments and has no + // `enabled` option - OrgSsoForm (the only thing that calls it) only + // mounts once canManageSso is true, which is what actually keeps the + // query from firing for a non-admin. See the "does not render" test + // below and the comment on the page component. + expect(useSsoConfig).toHaveBeenCalledWith(); + expect(screen.getByRole('heading', { name: /sso/i })).toBeInTheDocument(); + expect(screen.getByLabelText(/issuer url/i)).toBeInTheDocument(); + expect(screen.getByLabelText(/client id/i)).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument(); + }); + + it('does not render the SSO config form for a regular member, and never mounts the query', () => { + vi.mocked(usePermissions).mockReturnValue({ + isSystemAdmin: false, + orgRole: 'member', + isLoading: false, + } as ReturnType); + vi.mocked(useSsoConfig).mockReturnValue({ + config: undefined, + isLoading: false, + error: null, + updateConfig: vi.fn(), + isSaving: false, + } as ReturnType); + + render(); + + expect(screen.queryByRole('heading', { name: /sso/i })).not.toBeInTheDocument(); + // OrgSsoForm - the only component that calls useSsoConfig() - is never + // rendered for a non-admin, so the hook (and its query) never mounts. + expect(useSsoConfig).not.toHaveBeenCalled(); + }); + + it('surfaces a save error instead of an unhandled rejection when updateConfig fails', async () => { + const user = userEvent.setup(); + const updateConfig = vi.fn().mockRejectedValue(new Error('network error')); + vi.mocked(usePermissions).mockReturnValue({ + isSystemAdmin: false, + orgRole: 'admin', + isLoading: false, + } as ReturnType); + vi.mocked(useSsoConfig).mockReturnValue({ + config: undefined, + isLoading: false, + error: null, + updateConfig, + isSaving: false, + } as ReturnType); + + render(); + + await user.click(screen.getByRole('button', { name: /save/i })); + + expect(updateConfig).toHaveBeenCalledTimes(1); + expect(await screen.findByRole('alert')).toHaveTextContent(/failed to save configuration/i); + }); + + it('omits clientSecret from the update payload when the secret field is left blank', async () => { + const user = userEvent.setup(); + const updateConfig = vi.fn().mockResolvedValue(undefined); + vi.mocked(usePermissions).mockReturnValue({ + isSystemAdmin: false, + orgRole: 'admin', + isLoading: false, + } as ReturnType); + vi.mocked(useSsoConfig).mockReturnValue({ + config: { + issuerUrl: 'https://idp.example.com', + clientId: 'client-123', + hasClientSecret: true, + allowedDomains: ['example.com'], + enforceSso: false, + }, + isLoading: false, + error: null, + updateConfig, + isSaving: false, + } as ReturnType); + + render(); + + // Secret field is left untouched - a secret is already configured + // (hasClientSecret: true) but the user isn't rotating it. + await user.click(screen.getByRole('button', { name: /save/i })); + + expect(updateConfig).toHaveBeenCalledTimes(1); + const payload = updateConfig.mock.calls[0][0]; + expect(payload).not.toHaveProperty('clientSecret'); + expect(payload).toEqual({ + issuerUrl: 'https://idp.example.com', + clientId: 'client-123', + allowedDomains: ['example.com'], + enforceSso: false, + }); + }); + + it('includes the typed clientSecret and normalizes allowedDomains when the secret field is filled in', async () => { + const user = userEvent.setup(); + const updateConfig = vi.fn().mockResolvedValue(undefined); + vi.mocked(usePermissions).mockReturnValue({ + isSystemAdmin: false, + orgRole: 'admin', + isLoading: false, + } as ReturnType); + vi.mocked(useSsoConfig).mockReturnValue({ + config: undefined, + isLoading: false, + error: null, + updateConfig, + isSaving: false, + } as ReturnType); + + render(); + + await user.type(screen.getByLabelText(/issuer url/i), 'https://idp.example.com'); + await user.type(screen.getByLabelText(/client id/i), 'client-123'); + await user.type( + screen.getByLabelText(/allowed domains/i), + ' example.com , foo.com ,,bar.com ' + ); + await user.type(screen.getByLabelText(/client secret/i), 'shh-secret'); + await user.click(screen.getByRole('button', { name: /save/i })); + + expect(updateConfig).toHaveBeenCalledTimes(1); + expect(updateConfig).toHaveBeenCalledWith({ + issuerUrl: 'https://idp.example.com', + clientId: 'client-123', + allowedDomains: ['example.com', 'foo.com', 'bar.com'], + enforceSso: false, + clientSecret: 'shh-secret', + }); + }); + + it('does not wipe an in-progress edit when useSsoConfig returns a new config object every render (regression)', async () => { + const user = userEvent.setup(); + vi.mocked(usePermissions).mockReturnValue({ + isSystemAdmin: false, + orgRole: 'admin', + isLoading: false, + } as ReturnType); + + // The real useSsoConfig() rebuilds `config` via stripClientSecret's + // object spread on every call, so it hands back a new reference every + // render even when the data hasn't changed. mockReturnValue (used by + // the other tests above) instead returns the SAME reference every + // call, which is exactly why they don't exercise this bug - + // mockImplementation is required here to reproduce it. + vi.mocked(useSsoConfig).mockImplementation( + () => + ({ + config: { + issuerUrl: 'https://idp.example.com', + clientId: 'client-123', + hasClientSecret: false, + allowedDomains: ['example.com'], + enforceSso: false, + }, + isLoading: false, + error: null, + updateConfig: vi.fn(), + isSaving: false, + }) as ReturnType + ); + + render(); + + const issuerInput = screen.getByLabelText(/issuer url/i) as HTMLInputElement; + expect(issuerInput).toHaveValue('https://idp.example.com'); + + await user.clear(issuerInput); + await user.type(issuerInput, 'https://changed.example.com'); + + // Each keystroke re-renders OrgSsoForm, which calls useSsoConfig() + // again and gets a fresh `config` object back. Without a hydrate-once + // guard, the sync effect would see `config` as "changed" on every one + // of those re-renders and reset the field back to the loaded value, + // reverting the edit as it's typed. + expect(issuerInput).toHaveValue('https://changed.example.com'); + }); +});