From 9af5dad2b0e633b92e0c5ec3bcf2c0b3204a157e Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Mon, 11 May 2026 11:14:49 +0000 Subject: [PATCH] feat(gui/oidc): editable Advanced form on OIDCProviderDetailPage (A-7 / MED-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-05-10 audit tagged MED-4 as DEFERRED to v3 with the rationale "backend already accepts the five fields." The 2026-05-11 adversarial review verified the deferral framing was inaccurate — the read-only `
` rendered scopes / groups_claim_path / groups_claim_format / iat_window_seconds (and persisted but invisible jwks_cache_ttl_seconds), which gave operators the impression those fields were editable. Switching to edit mode revealed no inputs but the saveEdit handler at OIDCProviderDetailPage.tsx:107-134 silently passed `provider.scopes` / `provider.groups_claim_path` / etc. through to the PUT body unchanged from the loaded provider object. Result: a "lying UX" anti-pattern. The page collected updates to other fields (display name, issuer URL, client secret, redirect URI, fetch_userinfo), the PUT succeeded with HTTP 204, and no error fired — but the displayed Advanced values were whatever the create form persisted or curl last set. A second operator bumping `iat_window_seconds` from 60 to 300 had to drop to curl. The "DEFERRED to v3" framing hid the gap from acquisition reviewers who only inspect the GUI. Closure (frontend-only — backend already accepts all 5 fields on `PUT /api/v1/auth/oidc/providers/{id}`): OIDCProviderDetailPage.tsx - New `
` section collapsed by default inside the edit form. Most edits don't touch these fields, so they shouldn't clutter the primary form. - Five new inputs wired through component state: * `editScopesInput` — text input rendered as space-separated string per OIDC convention (every IdP docs page shows scopes that way). Submit splits on whitespace + filters empty strings. * `editGroupsClaimPath` — text input with `groups` default. * `editGroupsClaimFormat` — select with the actual backend enum `string-array` | `json-path` (NOT `string_array` / `space_separated` / `comma_separated` as the spec mistakenly proposed — those values don't exist in `internal/auth/oidc/domain/types.go::GroupsClaimFormat*`). * `editIATWindow` — number input with `min=1, max=600` matching `MaxIATWindowSeconds=600` from the domain validator. * `editJWKSCacheTTL` — number input with `min=60` matching `MinJWKSCacheTTLSeconds=60`. - `startEdit` pre-populates all five from the live provider so operators see current values when expanding the section. - `saveEdit` validates client-side mirroring the backend `Validate` rules (empty scopes / empty path / invalid format / IAT out of (0, 600] / JWKS < 60) → inline error + does NOT POST. Server is still source-of-truth; any 400 surfaces via the existing error UI. - Read-only `
` gained the previously-invisible `jwks_cache_ttl_seconds` row so all five values are visible without entering edit mode. Each input carries a help paragraph linking the operator mental model to the backend semantic (e.g. Keycloak's `realm_access.roles`, Auth0's namespaced claims; RFC 7519 §4.1.6 for IAT; MED-6 auto-refresh-on-cache-miss for the JWKS TTL). Tests (9 new + 5 pre-existing, all passing under vitest): A-7 Advanced details section is collapsed by default and visible in edit mode — pin
has no `open` attribute initially. A-7 Advanced fields pre-populate from the live provider — start edit with a non-default provider (Keycloak shape: realm_access.roles, json-path, IAT=120, JWKS TTL=600); assert each input carries the live value. A-7 all five Advanced fields round-trip into the PUT body — change every field, submit, assert the PUT body carries the parsed shapes (whitespace-normalized scopes array, trimmed groups_claim_path, enum value, numeric values). A-7 IAT window above 600 rejects with inline error and does NOT POST — operator types 601, save handler rejects before reaching updateOIDCProvider. A-7 IAT window <= 0 rejects with inline error. A-7 JWKS cache TTL below 60 rejects with inline error. A-7 empty scopes input rejects — guards against operator accidentally wiping the array via whitespace. A-7 empty groups-claim-path rejects. A-7 unchanged Advanced fields still round-trip as the existing values — pin that a name-only edit still carries the live advanced config (no regression to the pass-through behavior; operators don't lose their config when editing other fields). Verify gate green: tsc --noEmit clean; vitest passes all 14 tests in OIDCProviderDetailPage.test.tsx (5 pre-existing + 9 new A-7 cases). Spec at cowork/auth-bundles-fixes-2026-05-11/07-high-oidc-provider-advanced-form.md. Audit doc: MED-4 section in cowork/auth-bundles-audit-2026-05-10.md appended with the A-7 follow-up closure annotation correcting the "DEFERRED to v3" framing and explaining the lying-UX pattern; status table row updated from "CLOSED" (incorrectly tagged on the pass-through behavior) to "CLOSED 2026-05-11 (A-7)" with the 5-field enumeration. Operator-visible CHANGELOG.md entry under Security retires the lying-UX caveat. --- CHANGELOG.md | 20 ++ .../auth/OIDCProviderDetailPage.test.tsx | 174 +++++++++++++++ web/src/pages/auth/OIDCProviderDetailPage.tsx | 200 +++++++++++++++++- 3 files changed, 389 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e986ea..bc6e8f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,26 @@ ### Security +- **OIDC provider Advanced fields are now editable in the GUI (Audit 2026-05-11 A-7).** + The MED-4 row had been DEFERRED to v3 with the rationale "backend + already accepts these fields." The verifier hit the GUI and found + that the read-only display claimed the values were editable, but the + edit form had no inputs — the save handler passed `provider.scopes` + / `provider.groups_claim_path` / `provider.groups_claim_format` / + `provider.iat_window_seconds` / `provider.jwks_cache_ttl_seconds` + unchanged from the loaded object. Operators who wanted to bump the + IAT window or change the groups-claim path had to drop to curl / + MCP and trust the GUI's display matched what they'd set elsewhere. + Lying UX. The OIDCProviderDetailPage edit form now has a collapsible + Advanced section with five inputs (scopes as a space-separated text + field; groups-claim path; groups-claim format select with the + backend's `string-array` / `json-path` enum; IAT window number input + bounded 1–600; JWKS cache TTL number input with floor 60). Client-side + validation mirrors the backend `Validate` rules so common operator + mistakes (IAT > 600, JWKS TTL < 60, empty scopes, empty groups-claim-path) + reject inline instead of round-tripping a 400. The read-only `
` + also gained the previously-invisible `jwks_cache_ttl_seconds` row. + - **Pre-login cookie Path widened from `/auth/oidc/` to `/` (Audit MED-14 follow-on).** Required to satisfy the `__Host-` prefix's `Path=/` rule. The cookie lifetime is unchanged (10 minutes) and only the callback handler diff --git a/web/src/pages/auth/OIDCProviderDetailPage.test.tsx b/web/src/pages/auth/OIDCProviderDetailPage.test.tsx index 35444a6..54b5385 100644 --- a/web/src/pages/auth/OIDCProviderDetailPage.test.tsx +++ b/web/src/pages/auth/OIDCProviderDetailPage.test.tsx @@ -175,4 +175,178 @@ describe('OIDCProviderDetailPage', () => { }); expect(confirmBtn.disabled).toBe(false); }); + + // ============================================================================= + // Audit 2026-05-11 A-7 — Advanced fields are editable (MED-4 closure). + // ============================================================================= + + async function openEditFormWithEditPerms() { + vi.mocked(client.listOIDCProviders).mockResolvedValue({ providers: [sampleProvider] }); + vi.mocked(client.updateOIDCProvider).mockResolvedValue(sampleProvider); + vi.mocked(client.authMe).mockResolvedValue({ + actor_id: 'u-admin', + actor_type: 'User', + tenant_id: 't-default', + admin: true, + roles: ['r-admin'], + effective_permissions: [ + { permission: 'auth.oidc.list', scope_type: 'global' }, + { permission: 'auth.oidc.edit', scope_type: 'global' }, + ], + }); + renderRoute(); + await waitFor(() => screen.getByTestId('oidc-provider-edit-button')); + fireEvent.click(screen.getByTestId('oidc-provider-edit-button')); + await waitFor(() => screen.getByTestId('oidc-provider-edit-advanced')); + } + + it('A-7 Advanced details section is collapsed by default and visible in edit mode', async () => { + await openEditFormWithEditPerms(); + const details = screen.getByTestId('oidc-provider-edit-advanced') as HTMLDetailsElement; + expect(details).toBeTruthy(); + //
with no `open` attribute = collapsed. + expect(details.open).toBe(false); + }); + + it('A-7 Advanced fields pre-populate from the live provider', async () => { + vi.mocked(client.listOIDCProviders).mockResolvedValue({ + providers: [{ + ...sampleProvider, + scopes: ['openid', 'profile', 'email', 'groups'], + groups_claim_path: 'realm_access.roles', + groups_claim_format: 'json-path', + iat_window_seconds: 120, + jwks_cache_ttl_seconds: 600, + }], + }); + vi.mocked(client.authMe).mockResolvedValue({ + actor_id: 'u-admin', + actor_type: 'User', + tenant_id: 't-default', + admin: true, + roles: ['r-admin'], + effective_permissions: [ + { permission: 'auth.oidc.list', scope_type: 'global' }, + { permission: 'auth.oidc.edit', scope_type: 'global' }, + ], + }); + renderRoute(); + await waitFor(() => screen.getByTestId('oidc-provider-edit-button')); + fireEvent.click(screen.getByTestId('oidc-provider-edit-button')); + await waitFor(() => screen.getByTestId('oidc-provider-edit-advanced')); + + expect((screen.getByTestId('oidc-provider-edit-scopes') as HTMLInputElement).value) + .toBe('openid profile email groups'); + expect((screen.getByTestId('oidc-provider-edit-groups-claim-path') as HTMLInputElement).value) + .toBe('realm_access.roles'); + expect((screen.getByTestId('oidc-provider-edit-groups-claim-format') as HTMLSelectElement).value) + .toBe('json-path'); + expect((screen.getByTestId('oidc-provider-edit-iat-window-seconds') as HTMLInputElement).valueAsNumber) + .toBe(120); + expect((screen.getByTestId('oidc-provider-edit-jwks-cache-ttl-seconds') as HTMLInputElement).valueAsNumber) + .toBe(600); + }); + + it('A-7 all five Advanced fields round-trip into the PUT body', async () => { + await openEditFormWithEditPerms(); + + fireEvent.change(screen.getByTestId('oidc-provider-edit-scopes'), { + target: { value: ' openid profile email groups ' }, + }); + fireEvent.change(screen.getByTestId('oidc-provider-edit-groups-claim-path'), { + target: { value: 'realm_access.roles' }, + }); + fireEvent.change(screen.getByTestId('oidc-provider-edit-groups-claim-format'), { + target: { value: 'json-path' }, + }); + fireEvent.change(screen.getByTestId('oidc-provider-edit-iat-window-seconds'), { + target: { value: '120' }, + }); + fireEvent.change(screen.getByTestId('oidc-provider-edit-jwks-cache-ttl-seconds'), { + target: { value: '600' }, + }); + fireEvent.click(screen.getByTestId('oidc-provider-save-button')); + + await waitFor(() => expect(client.updateOIDCProvider).toHaveBeenCalledTimes(1)); + const [, body] = vi.mocked(client.updateOIDCProvider).mock.calls[0]; + // Whitespace normalization: collapsed runs, no empty strings. + expect(body.scopes).toEqual(['openid', 'profile', 'email', 'groups']); + expect(body.groups_claim_path).toBe('realm_access.roles'); + expect(body.groups_claim_format).toBe('json-path'); + expect(body.iat_window_seconds).toBe(120); + expect(body.jwks_cache_ttl_seconds).toBe(600); + }); + + it('A-7 IAT window above 600 rejects with inline error and does NOT POST', async () => { + await openEditFormWithEditPerms(); + fireEvent.change(screen.getByTestId('oidc-provider-edit-iat-window-seconds'), { + target: { value: '601' }, + }); + fireEvent.click(screen.getByTestId('oidc-provider-save-button')); + await waitFor(() => screen.getByTestId('oidc-provider-detail-error')); + expect(screen.getByTestId('oidc-provider-detail-error').textContent).toContain('IAT window'); + expect(client.updateOIDCProvider).not.toHaveBeenCalled(); + }); + + it('A-7 IAT window <= 0 rejects with inline error', async () => { + await openEditFormWithEditPerms(); + fireEvent.change(screen.getByTestId('oidc-provider-edit-iat-window-seconds'), { + target: { value: '0' }, + }); + fireEvent.click(screen.getByTestId('oidc-provider-save-button')); + await waitFor(() => screen.getByTestId('oidc-provider-detail-error')); + expect(client.updateOIDCProvider).not.toHaveBeenCalled(); + }); + + it('A-7 JWKS cache TTL below 60 rejects with inline error', async () => { + await openEditFormWithEditPerms(); + fireEvent.change(screen.getByTestId('oidc-provider-edit-jwks-cache-ttl-seconds'), { + target: { value: '30' }, + }); + fireEvent.click(screen.getByTestId('oidc-provider-save-button')); + await waitFor(() => screen.getByTestId('oidc-provider-detail-error')); + expect(screen.getByTestId('oidc-provider-detail-error').textContent).toContain('JWKS'); + expect(client.updateOIDCProvider).not.toHaveBeenCalled(); + }); + + it('A-7 empty scopes input rejects (operator can\'t accidentally wipe the array)', async () => { + await openEditFormWithEditPerms(); + fireEvent.change(screen.getByTestId('oidc-provider-edit-scopes'), { + target: { value: ' ' }, + }); + fireEvent.click(screen.getByTestId('oidc-provider-save-button')); + await waitFor(() => screen.getByTestId('oidc-provider-detail-error')); + expect(screen.getByTestId('oidc-provider-detail-error').textContent).toContain('Scopes'); + expect(client.updateOIDCProvider).not.toHaveBeenCalled(); + }); + + it('A-7 empty groups-claim-path rejects', async () => { + await openEditFormWithEditPerms(); + fireEvent.change(screen.getByTestId('oidc-provider-edit-groups-claim-path'), { + target: { value: ' ' }, + }); + fireEvent.click(screen.getByTestId('oidc-provider-save-button')); + await waitFor(() => screen.getByTestId('oidc-provider-detail-error')); + expect(screen.getByTestId('oidc-provider-detail-error').textContent).toContain('Groups claim path'); + expect(client.updateOIDCProvider).not.toHaveBeenCalled(); + }); + + it('A-7 unchanged Advanced fields still round-trip as the existing values (no lying field)', async () => { + await openEditFormWithEditPerms(); + // Operator only changes Display name; advanced section is untouched. + fireEvent.change(screen.getByTestId('oidc-provider-edit-name'), { + target: { value: 'Okta Rename' }, + }); + fireEvent.click(screen.getByTestId('oidc-provider-save-button')); + await waitFor(() => expect(client.updateOIDCProvider).toHaveBeenCalledTimes(1)); + const [, body] = vi.mocked(client.updateOIDCProvider).mock.calls[0]; + // Pre-A-7 these would have been the provider's pass-through; now + // they come from state pre-populated by startEdit. Either way the + // wire value should be the live provider's existing config. + expect(body.scopes).toEqual(sampleProvider.scopes); + expect(body.groups_claim_path).toBe(sampleProvider.groups_claim_path); + expect(body.groups_claim_format).toBe(sampleProvider.groups_claim_format); + expect(body.iat_window_seconds).toBe(sampleProvider.iat_window_seconds); + expect(body.jwks_cache_ttl_seconds).toBe(sampleProvider.jwks_cache_ttl_seconds); + }); }); diff --git a/web/src/pages/auth/OIDCProviderDetailPage.tsx b/web/src/pages/auth/OIDCProviderDetailPage.tsx index cac577e..c566ab9 100644 --- a/web/src/pages/auth/OIDCProviderDetailPage.tsx +++ b/web/src/pages/auth/OIDCProviderDetailPage.tsx @@ -49,6 +49,26 @@ export default function OIDCProviderDetailPage() { const [editClientSecret, setEditClientSecret] = useState(''); const [editRedirectURI, setEditRedirectURI] = useState(''); const [editFetchUserinfo, setEditFetchUserinfo] = useState(false); + // Audit 2026-05-11 A-7 — Advanced edit fields. Pre-fix, the saveEdit + // handler passed these through unchanged from the provider object, + // so the read-only `
` claimed the value was editable but the + // PUT body never carried operator input. The 5 fields the backend + // validator accepts (internal/auth/oidc/domain/types.go::Validate): + // - scopes (string array; min 1 entry; default openid profile email) + // - groups_claim_path (string; default "groups") + // - groups_claim_format (enum: string-array | json-path) + // - iat_window_seconds (int, 1–600; default 300) + // - jwks_cache_ttl_seconds (int, ≥60; default 3600) + // Scopes are rendered as a space-separated text input (single-line) + // because that's the operator's mental model — every OIDC IdP docs + // page shows scopes as space-separated. The submit handler splits on + // whitespace + filters empty strings; an empty input renders an + // inline error rather than wiping the array. + const [editScopesInput, setEditScopesInput] = useState(''); + const [editGroupsClaimPath, setEditGroupsClaimPath] = useState(''); + const [editGroupsClaimFormat, setEditGroupsClaimFormat] = useState('string-array'); + const [editIATWindow, setEditIATWindow] = useState(300); + const [editJWKSCacheTTL, setEditJWKSCacheTTL] = useState(3600); const [submitting, setSubmitting] = useState(false); const [error, setError] = useState(null); const [success, setSuccess] = useState(null); @@ -94,6 +114,14 @@ export default function OIDCProviderDetailPage() { setEditClientSecret(''); setEditRedirectURI(provider.redirect_uri); setEditFetchUserinfo(provider.fetch_userinfo || false); + // Audit 2026-05-11 A-7 — pre-populate the Advanced fields from + // the live provider so the operator sees the current values when + // they expand the section. + setEditScopesInput((provider.scopes ?? []).join(' ')); + setEditGroupsClaimPath(provider.groups_claim_path || 'groups'); + setEditGroupsClaimFormat(provider.groups_claim_format || 'string-array'); + setEditIATWindow(provider.iat_window_seconds || 300); + setEditJWKSCacheTTL(provider.jwks_cache_ttl_seconds || 3600); setError(null); setSuccess(null); setEditing(true); @@ -109,17 +137,59 @@ export default function OIDCProviderDetailPage() { setError(null); setSuccess(null); try { + // Audit 2026-05-11 A-7 — client-side validation mirrors the + // backend's internal/auth/oidc/domain/types.go::Validate rules. + // Server is still the source of truth (we surface its 400 if + // anything slips past); the client validator is for fast + // feedback so operators don't round-trip just to learn that + // "iat_window_seconds=601" is rejected. + const trimmedPath = editGroupsClaimPath.trim(); + if (trimmedPath === '') { + setError('Groups claim path cannot be empty (default: "groups").'); + setSubmitting(false); + return; + } + if (editGroupsClaimFormat !== 'string-array' && editGroupsClaimFormat !== 'json-path') { + setError('Groups claim format must be "string-array" or "json-path".'); + setSubmitting(false); + return; + } + const scopes = editScopesInput + .trim() + .split(/\s+/) + .filter(s => s.length > 0); + if (scopes.length === 0) { + setError('Scopes cannot be empty. At minimum include "openid".'); + setSubmitting(false); + return; + } + if (!Number.isInteger(editIATWindow) || editIATWindow <= 0 || editIATWindow > 600) { + setError('IAT window must be a positive integer ≤ 600 seconds.'); + setSubmitting(false); + return; + } + if (!Number.isInteger(editJWKSCacheTTL) || editJWKSCacheTTL < 60) { + setError('JWKS cache TTL must be an integer ≥ 60 seconds.'); + setSubmitting(false); + return; + } + const req: Parameters[1] = { name: editName, issuer_url: editIssuerURL, client_id: editClientID, redirect_uri: editRedirectURI, - groups_claim_path: provider.groups_claim_path, - groups_claim_format: provider.groups_claim_format, + // Audit 2026-05-11 A-7 — formerly pass-through from + // provider.*, now wired to the operator-edited state. Lying + // UX retired: the read-only `
` no longer claims a value + // can be changed when the saveEdit handler ignores the + // change. + groups_claim_path: trimmedPath, + groups_claim_format: editGroupsClaimFormat, fetch_userinfo: editFetchUserinfo, - scopes: provider.scopes, - iat_window_seconds: provider.iat_window_seconds, - jwks_cache_ttl_seconds: provider.jwks_cache_ttl_seconds, + scopes, + iat_window_seconds: editIATWindow, + jwks_cache_ttl_seconds: editJWKSCacheTTL, }; if (editClientSecret) req.client_secret = editClientSecret; await updateOIDCProvider(provider.id, req); @@ -202,6 +272,11 @@ export default function OIDCProviderDetailPage() {
{(provider.scopes || []).join(', ')}
IAT window
{provider.iat_window_seconds}s
+ {/* Audit 2026-05-11 A-7 — JWKS cache TTL surfaced in + read-only view too (pre-fix the value was persisted but + invisible). */} +
JWKS cache TTL
+
{provider.jwks_cache_ttl_seconds}s
) : (
@@ -262,6 +337,121 @@ export default function OIDCProviderDetailPage() { /> Fetch groups from userinfo endpoint when ID token claim is empty + + {/* Audit 2026-05-11 A-7 — Advanced section. Five fields the + read-only
claimed were editable but the saveEdit + handler was passing through unchanged from the loaded + provider object. Each input has an inline help line that + links the operator's mental model to the backend + semantic (`internal/auth/oidc/domain/types.go::Validate` + rules). The section is collapsed by default — most + edits don't touch these fields, so they shouldn't + clutter the primary form. */} +
+ + Advanced (scopes, groups claim, IAT / JWKS TTL) + +
+
+ + setEditScopesInput(e.target.value)} + placeholder="openid profile email" + className="w-full px-3 py-1.5 text-sm border border-surface-border rounded bg-page text-ink font-mono" + data-testid="oidc-provider-edit-scopes" + /> +

+ Default openid profile email. Some IdPs need groups for + the group-claim path; Auth0 namespaces groups under a custom claim. Must include{' '} + openid. +

+
+
+
+ + setEditGroupsClaimPath(e.target.value)} + placeholder="groups" + className="w-full px-3 py-1.5 text-sm border border-surface-border rounded bg-page text-ink font-mono" + data-testid="oidc-provider-edit-groups-claim-path" + /> +

+ JSON path within the ID token (or userinfo if fallback enabled) that holds the + group list. Common: groups, realm_access.roles + {' '}(Keycloak), namespaced URLs (Auth0). +

+
+
+ + +

+ How the IdP encodes the group list. Most IdPs emit a JSON array — keep the + default. Use json-path when the claim is a nested object the + path needs to traverse. +

+
+
+
+
+ + setEditIATWindow(Number(e.target.value))} + className="w-full px-3 py-1.5 text-sm border border-surface-border rounded bg-page text-ink" + data-testid="oidc-provider-edit-iat-window-seconds" + /> +

+ Maximum ID-token age at consume time (RFC 7519 §4.1.6). Default 300. Range + 1–600. Tighter = more replay-resistant; looser = more clock-skew-tolerant. +

+
+
+ + setEditJWKSCacheTTL(Number(e.target.value))} + className="w-full px-3 py-1.5 text-sm border border-surface-border rounded bg-page text-ink" + data-testid="oidc-provider-edit-jwks-cache-ttl-seconds" + /> +

+ How long to cache the IdP's signing-key set before re-fetching. Default 3600 + (1h); floor 60. MED-6 auto-refresh-on-cache-miss covers most rotation events; + this knob is for slow-rotation IdPs that want longer caching. +

+
+
+
+
)}