fix(web): block close on incomplete EMA config, not just an invalid issuer (closes #1546) (#1547)

Follow-up to #1545. The close-block only fired for an invalid issuer, so an
EMA config enabled with a valid issuer but a blank client ID was still silently
dropped on close by the persist gate — the same no-feedback bug, different
field.

- validateClientSettings gains a requireComplete mode that also flags the
  required IdP fields (issuer, clientId) when blank, with distinct per-field
  messages. Inline (as-you-type) validation is unchanged — required-but-blank
  errors only surface on reveal, so the form never nags about not-yet-filled
  fields.
- canPersistClientSettingsDraft is redefined in terms of that mode, so the
  persist gate and the close-time field errors share one source of truth.
- ClientSettingsModal.handleClose now blocks + reveals on ANY require-complete
  error (invalid issuer OR blank required field) and keeps the modal open. The
  form prop revealIssuerError becomes revealErrors and surfaces the client-ID
  error too. Escape hatch: disable enterprise IdP (an empty issuer no longer
  closes, since that's itself incomplete).

Tests cover the blank-clientId close path, fix-each-field, and disable-EMA
escape, plus requireComplete validation.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx

@@ -10,6 +10,8 @@ import {
 import {
   EMPTY_CLIENT_SETTINGS,
   ISSUER_URL_ERROR,
+  ISSUER_REQUIRED_ERROR,
+  CLIENT_ID_REQUIRED_ERROR,
   type ClientSettingsFormValues,
 } from "./clientSettingsValues";
 
@@ -139,7 +141,7 @@ describe("ClientSettingsForm EMA IdP session", () => {
     expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument();
   });
 
-  it("reveals the issuer error without blur when revealIssuerError is set", () => {
+  it("reveals the issuer error without blur when revealErrors is set", () => {
     renderWithMantine(
       <ClientSettingsForm
         settings={{
@@ -151,7 +153,7 @@ describe("ClientSettingsForm EMA IdP session", () => {
         onExpandedSectionsChange={vi.fn()}
         onSettingsChange={vi.fn()}
         emaIdpLoginState="none"
-        revealIssuerError
+        revealErrors
       />,
     );
 
@@ -160,23 +162,63 @@ describe("ClientSettingsForm EMA IdP session", () => {
     expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument();
   });
 
-  it("shows no error for a valid issuer even when revealIssuerError is set", () => {
+  it("reveals required errors for blank issuer and client ID when revealErrors is set", () => {
+    renderWithMantine(
+      <ClientSettingsForm
+        settings={{ ...EMPTY_CLIENT_SETTINGS, emaEnabled: true }}
+        expandedSections={["ema"]}
+        onExpandedSectionsChange={vi.fn()}
+        onSettingsChange={vi.fn()}
+        emaIdpLoginState="none"
+        revealErrors
+      />,
+    );
+
+    // Both required IdP fields are blank; a close/save attempt surfaces them
+    // instead of silently dropping the config.
+    expect(screen.getByText(ISSUER_REQUIRED_ERROR)).toBeInTheDocument();
+    expect(screen.getByText(CLIENT_ID_REQUIRED_ERROR)).toBeInTheDocument();
+  });
+
+  it("does not show required errors for blank fields before revealErrors", () => {
+    renderWithMantine(
+      <ClientSettingsForm
+        settings={{ ...EMPTY_CLIENT_SETTINGS, emaEnabled: true }}
+        expandedSections={["ema"]}
+        onExpandedSectionsChange={vi.fn()}
+        onSettingsChange={vi.fn()}
+        emaIdpLoginState="none"
+      />,
+    );
+
+    // Inline validation must not nag about not-yet-filled required fields.
+    expect(screen.queryByText(ISSUER_REQUIRED_ERROR)).not.toBeInTheDocument();
+    expect(
+      screen.queryByText(CLIENT_ID_REQUIRED_ERROR),
+    ).not.toBeInTheDocument();
+  });
+
+  it("shows no error for a valid issuer even when revealErrors is set", () => {
     renderWithMantine(
       <ClientSettingsForm
         settings={{
           ...EMPTY_CLIENT_SETTINGS,
           emaEnabled: true,
           issuer: "https://idp.test",
+          clientId: "client-1",
         }}
         expandedSections={["ema"]}
         onExpandedSectionsChange={vi.fn()}
         onSettingsChange={vi.fn()}
         emaIdpLoginState="none"
-        revealIssuerError
+        revealErrors
       />,
     );
 
     expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument();
+    expect(
+      screen.queryByText(CLIENT_ID_REQUIRED_ERROR),
+    ).not.toBeInTheDocument();
   });
 
   it("shows no issuer error for a valid URL", () => {
@@ -251,7 +293,7 @@ describe("ClientSettingsForm EMA IdP session", () => {
     ).not.toBeInTheDocument();
   });
 
-  it("reveals the CIMD URL error without blur when revealClientMetadataUrlError is set", () => {
+  it("reveals the CIMD URL error without blur when revealErrors is set", () => {
     renderWithMantine(
       <ClientSettingsForm
         settings={{
@@ -263,7 +305,7 @@ describe("ClientSettingsForm EMA IdP session", () => {
         onExpandedSectionsChange={vi.fn()}
         onSettingsChange={vi.fn()}
         emaIdpLoginState="none"
-        revealClientMetadataUrlError
+        revealErrors
       />,
     );
 

clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.tsx

@@ -30,16 +30,12 @@ export interface ClientSettingsFormProps {
   emaIdpLoginState?: EmaIdpLoginState;
   onEmaIdpLogout?: () => void;
   /**
-   * Force the issuer error to show even before the field is blurred. The parent
-   * sets this when a save/close is attempted with an invalid issuer, so the user
-   * isn't left with a silently-dropped value and no explanation.
+   * Force all field errors to show, including required-but-blank fields (EMA's
+   * issuer/clientId, CIMD's metadata URL). The parent sets this when a
+   * save/close is attempted with an incomplete or invalid config, so nothing is
+   * silently dropped without explanation.
    */
-  revealIssuerError?: boolean;
-  /**
-   * Force the CIMD metadata URL error to show even before the field is blurred.
-   * Same contract as {@link revealIssuerError} for the CIMD URL field.
-   */
-  revealClientMetadataUrlError?: boolean;
+  revealErrors?: boolean;
 }
 
 const HintText = Text.withProps({
@@ -54,8 +50,7 @@ export function ClientSettingsForm({
   onSettingsChange,
   emaIdpLoginState = "none",
   onEmaIdpLogout,
-  revealIssuerError = false,
-  revealClientMetadataUrlError = false,
+  revealErrors = false,
 }: ClientSettingsFormProps) {
   function patch(partial: Partial<ClientSettingsFormValues>) {
     onSettingsChange((prev) => ({ ...prev, ...partial }));
@@ -64,23 +59,31 @@ export function ClientSettingsForm({
   // Defer the issuer error until the field has been blurred so it doesn't nag
   // mid-typing (e.g. while "https:/…" is still incomplete). Once touched it
   // updates live, so the error clears as soon as a valid URL is entered. The
-  // parent also forces it on via `revealIssuerError` when a close/save is
-  // attempted with an invalid value, so the value is never silently dropped
-  // without explanation. The persist gate (canPersistClientSettingsDraft)
-  // validates independently — an invalid issuer is never written regardless.
+  // parent forces all errors on via `revealErrors` when a close/save is
+  // attempted with an incomplete or invalid config — including the
+  // required-but-blank fields, which inline validation otherwise leaves silent.
+  // The persist gate (canPersistClientSettingsDraft) validates independently, so
+  // an incomplete/invalid config is never written regardless of these flags.
   const [issuerTouched, setIssuerTouched] = useState(false);
   const [clientMetadataUrlTouched, setClientMetadataUrlTouched] =
     useState(false);
 
-  const errors = validateClientSettings(settings);
-  const showIssuerError =
-    (issuerTouched || revealIssuerError) && errors.issuer
-      ? errors.issuer
+  // Inline errors flag only a filled-in-wrong field (issuer URL); the
+  // require-complete set adds the blank-required fields surfaced on reveal.
+  const inlineErrors = validateClientSettings(settings);
+  const revealedErrors = validateClientSettings(settings, {
+    requireComplete: true,
+  });
+  const showIssuerError = revealErrors
+    ? revealedErrors.issuer
+    : issuerTouched
+      ? inlineErrors.issuer
       : undefined;
-  const showClientMetadataUrlError =
-    (clientMetadataUrlTouched || revealClientMetadataUrlError) &&
-    errors.clientMetadataUrl
-      ? errors.clientMetadataUrl
+  const showClientIdError = revealErrors ? revealedErrors.clientId : undefined;
+  const showClientMetadataUrlError = revealErrors
+    ? revealedErrors.clientMetadataUrl
+    : clientMetadataUrlTouched
+      ? inlineErrors.clientMetadataUrl
       : undefined;
 
   const showIdpSession =
@@ -134,6 +137,7 @@ export function ClientSettingsForm({
                   description="Client id registered with your enterprise IdP."
                   value={settings.clientId}
                   onChange={(e) => patch({ clientId: e.currentTarget.value })}
+                  error={showClientIdError}
                   rightSectionPointerEvents="auto"
                   rightSection={
                     settings.clientId ? (

clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts

@@ -10,6 +10,9 @@ import {
   EMPTY_CLIENT_SETTINGS,
   formValuesToClientConfig,
   ISSUER_URL_ERROR,
+  ISSUER_REQUIRED_ERROR,
+  CLIENT_ID_REQUIRED_ERROR,
+  CLIENT_METADATA_URL_REQUIRED_ERROR,
   validateClientSettings,
 } from "./clientSettingsValues";
 
@@ -457,4 +460,92 @@ describe("clientSettingsValues", () => {
       }),
     ).toEqual({});
   });
+
+  it("validateClientSettings does not flag blank required fields by default", () => {
+    expect(
+      validateClientSettings({
+        emaEnabled: true,
+        issuer: "",
+        clientId: "",
+        clientSecret: "",
+        ...emptyCimd,
+      }),
+    ).toEqual({});
+  });
+
+  it("validateClientSettings with requireComplete flags blank issuer and clientId", () => {
+    expect(
+      validateClientSettings(
+        {
+          emaEnabled: true,
+          issuer: "",
+          clientId: "",
+          clientSecret: "",
+          ...emptyCimd,
+        },
+        { requireComplete: true },
+      ),
+    ).toEqual({
+      issuer: ISSUER_REQUIRED_ERROR,
+      clientId: CLIENT_ID_REQUIRED_ERROR,
+    });
+  });
+
+  it("validateClientSettings with requireComplete keeps the URL error over the required error", () => {
+    expect(
+      validateClientSettings(
+        {
+          emaEnabled: true,
+          issuer: "not-a-url",
+          clientId: "cid",
+          clientSecret: "",
+          ...emptyCimd,
+        },
+        { requireComplete: true },
+      ),
+    ).toEqual({ issuer: ISSUER_URL_ERROR });
+  });
+
+  it("validateClientSettings with requireComplete flags only the blank clientId when the issuer is valid", () => {
+    expect(
+      validateClientSettings(
+        {
+          emaEnabled: true,
+          issuer: "https://idp.test",
+          clientId: "",
+          clientSecret: "",
+          ...emptyCimd,
+        },
+        { requireComplete: true },
+      ),
+    ).toEqual({ clientId: CLIENT_ID_REQUIRED_ERROR });
+  });
+
+  it("validateClientSettings with requireComplete flags a blank CIMD URL", () => {
+    expect(
+      validateClientSettings(
+        {
+          emaEnabled: false,
+          issuer: "",
+          clientId: "",
+          clientSecret: "",
+          cimdEnabled: true,
+          clientMetadataUrl: "",
+        },
+        { requireComplete: true },
+      ),
+    ).toEqual({ clientMetadataUrl: CLIENT_METADATA_URL_REQUIRED_ERROR });
+  });
+
+  it("canPersistClientSettingsDraft blocks a blank clientId with a valid issuer", () => {
+    expect(
+      canPersistClientSettingsDraft({
+        emaEnabled: true,
+        issuer: "https://idp.test",
+        clientId: "",
+        clientSecret: "",
+        ...emptyCimd,
+      }),
+    ).toBe(false);
+  });
 });

clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts

@@ -7,35 +7,72 @@ import {
 /** Field-level error message for an issuer that is not an http(s) URL. */
 export const ISSUER_URL_ERROR =
   "Must be an http(s) URL, like https://idp.example.com";
+/** Field-level errors for a required field left blank while its feature is enabled. */
+export const ISSUER_REQUIRED_ERROR =
+  "Issuer is required when enterprise IdP configuration is enabled";
+export const CLIENT_ID_REQUIRED_ERROR =
+  "Client ID is required when enterprise IdP configuration is enabled";
+export const CLIENT_METADATA_URL_REQUIRED_ERROR =
+  "Metadata document URL is required when CIMD is enabled";
 
 /** Field-level validation errors for the client settings form. */
 export interface ClientSettingsErrors {
   issuer?: string;
+  clientId?: string;
   clientMetadataUrl?: string;
 }
 
+export interface ValidateClientSettingsOptions {
+  /**
+   * Also flag required fields left blank. Off by default so inline (as-you-type)
+   * validation only flags a field the user has actually filled in wrong; turned
+   * on when a save/close is attempted so an incomplete config is surfaced rather
+   * than silently dropped.
+   */
+  requireComplete?: boolean;
+}
+
 /**
- * Inline validation for the client settings form. Only flags fields that the
- * user has actually filled in — empty required fields are gated by
- * {@link canPersistClientSettingsDraft} rather than surfaced as errors.
+ * Validation for the client settings form. By default only flags fields the
+ * user has filled in wrong (a non-empty, non-http(s) issuer; a malformed CIMD
+ * metadata URL). With `requireComplete`, also flags the required fields of an
+ * enabled feature (EMA's issuer/clientId, CIMD's metadata URL) when they are
+ * blank — used at save/close time so an enabled-but-incomplete config never
+ * persists silently.
  */
 export function validateClientSettings(
   values: ClientSettingsFormValues,
+  options: ValidateClientSettingsOptions = {},
 ): ClientSettingsErrors {
   const errors: ClientSettingsErrors = {};
-  if (
-    values.emaEnabled &&
-    values.issuer.trim() !== "" &&
-    !isAbsoluteHttpUrl(values.issuer)
-  ) {
-    errors.issuer = ISSUER_URL_ERROR;
-  }
-  if (values.cimdEnabled && values.clientMetadataUrl.trim() !== "") {
-    const cimdError = getCimdClientMetadataUrlError(values.clientMetadataUrl);
-    if (cimdError) {
-      errors.clientMetadataUrl = cimdError;
+
+  if (values.emaEnabled) {
+    const issuer = values.issuer.trim();
+    if (issuer === "") {
+      if (options.requireComplete) errors.issuer = ISSUER_REQUIRED_ERROR;
+    } else if (!isAbsoluteHttpUrl(values.issuer)) {
+      errors.issuer = ISSUER_URL_ERROR;
+    }
+
+    if (options.requireComplete && values.clientId.trim() === "") {
+      errors.clientId = CLIENT_ID_REQUIRED_ERROR;
     }
   }
+
+  if (values.cimdEnabled) {
+    const url = values.clientMetadataUrl.trim();
+    if (url === "") {
+      if (options.requireComplete) {
+        errors.clientMetadataUrl = CLIENT_METADATA_URL_REQUIRED_ERROR;
+      }
+    } else {
+      const cimdError = getCimdClientMetadataUrlError(values.clientMetadataUrl);
+      if (cimdError) {
+        errors.clientMetadataUrl = cimdError;
+      }
+    }
+  }
+
   return errors;
 }
 
@@ -108,18 +145,18 @@ export function formValuesToClientConfig(
   return result;
 }
 
-/** Skip debounced persist while required fields are blank for enabled features. */
+/**
+ * Skip the debounced persist while an enabled feature's required fields are
+ * blank or invalid (EMA's issuer/clientId, CIMD's metadata URL). Defers to
+ * {@link validateClientSettings} in `requireComplete` mode so the persist gate
+ * and the close-time field errors can never drift: the same condition that
+ * blocks the write also surfaces the field-level errors that explain why.
+ */
 export function canPersistClientSettingsDraft(
   values: ClientSettingsFormValues,
 ): boolean {
-  if (values.emaEnabled) {
-    if (values.issuer.trim() === "" || values.clientId.trim() === "")
-      return false;
-  }
-  if (values.cimdEnabled) {
-    if (!values.clientMetadataUrl.trim()) return false;
-  }
-  // Defer to validateClientSettings so the persist gate and inline field errors
-  // can never drift — invalid values are never sent to the backend.
-  return Object.keys(validateClientSettings(values)).length === 0;
+  return (
+    Object.keys(validateClientSettings(values, { requireComplete: true }))
+      .length === 0
+  );
 }