Fix validation on account provisioning page to require secret (#50443)

**Related issue:** Resolves #50248 Clears the secret field with a clear error, forcing the user to re-enter it, on a URL change, and displays the actual server error strings rather than a generic error(though the server errors are currently largely unreachable via frontend since validation has been tightened up). # Checklist for submitter If some of the following don't apply, delete the relevant line. Unreleased bug so no changes file - [ ] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results  ## Summary by CodeRabbit * **New Features** * Added HTTPS validation for account provisioning token URLs. * Server-side validation errors now appear on the relevant form fields. * Update failures display helpful server-provided error messages. * **Bug Fixes** * Changing a token URL now clears masked secrets and requires the secret to be entered again. * Prevented form submission when required secret re-entry is missing.
2026-08-03 14:47:47 -04:00
parent 756295aabf
commit 3ea4304126
2 changed files with 248 additions and 18 deletions
@@ -4,10 +4,22 @@ import { screen, waitFor } from "@testing-library/react";
 import { createCustomRenderer, createMockRouter } from "test/test-utils";
 import createMockConfig from "__mocks__/configMock";
 import createMockLicense from "__mocks__/licenseMock";
+import configAPI from "services/entities/config";
+import { notify } from "components/ToastNotification";
 import { IAppConfigFormProps } from "pages/admin/OrgSettingsPage/cards/constants";

 import AccountProvisioning from "./AccountProvisioning";

+jest.mock("services/entities/config");
+jest.mock("components/ToastNotification", () => ({
+  notify: {
+    success: jest.fn(),
+    error: jest.fn(),
+    batch: jest.fn(),
+    dismiss: jest.fn(),
+  },
+}));
+
 const defaultProps: IAppConfigFormProps = {
  appConfig: createMockConfig({
    license: createMockLicense({ tier: "premium" }),
@@ -16,11 +28,30 @@ const defaultProps: IAppConfigFormProps = {
  router: createMockRouter(),
 };

+const savedConfigProps: IAppConfigFormProps = {
+  ...defaultProps,
+  appConfig: createMockConfig({
+    license: createMockLicense({ tier: "premium" }),
+    mdm: {
+      ...createMockConfig().mdm,
+      apple_account_provisioning: {
+        oauth_idp_token_url: "https://example.okta.com/oauth2/v1/token",
+        oauth_idp_client_id: "my-client-id",
+        oauth_idp_client_secret: "********",
+      },
+    },
+  }),
+};
+
 describe("AccountProvisioning", () => {
  const render = createCustomRenderer({
    withBackendMock: true,
  });

+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
  it("renders the section heading", () => {
    render(<AccountProvisioning {...defaultProps} />);
    expect(screen.getByText("Account provisioning")).toBeInTheDocument();
@@ -88,7 +119,23 @@ describe("AccountProvisioning", () => {
      await user.type(screen.getByLabelText(/token url/i), "not-a-url");
      await user.tab();
      await waitFor(() => {
-        expect(screen.getByText(/must be a valid url/i)).toBeInTheDocument();
+        expect(
+          screen.getByText(/must be a valid https url/i)
+        ).toBeInTheDocument();
+      });
+    });
+
+    it("shows an invalid URL error on blur when the URL is not https", async () => {
+      const { user } = render(<AccountProvisioning {...defaultProps} />);
+      await user.type(
+        screen.getByLabelText(/token url/i),
+        "http://example.okta.com/oauth2/v1/token"
+      );
+      await user.tab();
+      await waitFor(() => {
+        expect(
+          screen.getByText(/must be a valid https url/i)
+        ).toBeInTheDocument();
      });
    });

@@ -97,7 +144,9 @@ describe("AccountProvisioning", () => {
      await user.type(screen.getByLabelText(/token url/i), "not-a-url");
      await user.tab();
      await waitFor(() => {
-        expect(screen.getByText(/must be a valid url/i)).toBeInTheDocument();
+        expect(
+          screen.getByText(/must be a valid https url/i)
+        ).toBeInTheDocument();
      });
      // After the error shows, FormField replaces the label text with the error
      // message, so we locate the input by its placeholder instead.
@@ -111,7 +160,7 @@ describe("AccountProvisioning", () => {
      );
      await waitFor(() => {
        expect(
-          screen.queryByText(/must be a valid url/i)
+          screen.queryByText(/must be a valid https url/i)
        ).not.toBeInTheDocument();
      });
    });
@@ -164,7 +213,145 @@ describe("AccountProvisioning", () => {
      );
      await user.click(screen.getByRole("button", { name: /save/i }));
      await waitFor(() => {
-        expect(screen.getByText(/must be a valid url/i)).toBeInTheDocument();
+        expect(
+          screen.getByText(/must be a valid https url/i)
+        ).toBeInTheDocument();
+      });
+      expect(configAPI.update).not.toHaveBeenCalled();
+    });
+  });
+
+  describe("Editing the token URL of a saved configuration", () => {
+    it("clears the masked client secret and flags it for re-entry", async () => {
+      const { user } = render(<AccountProvisioning {...savedConfigProps} />);
+      await user.type(screen.getByLabelText(/token url/i), "x");
+
+      // the error replaces the "Client secret" label text
+      const secretInput = screen.getByLabelText(
+        /client secret must be re-entered/i
+      );
+      expect(secretInput).toHaveValue("");
+    });
+
+    it("does not clear a client secret the user has already re-entered", async () => {
+      const { user } = render(<AccountProvisioning {...savedConfigProps} />);
+      const secretInput = screen.getByLabelText(/client secret/i);
+      await user.clear(secretInput);
+      await user.type(secretInput, "new-secret");
+
+      await user.type(screen.getByLabelText(/token url/i), "x");
+
+      expect(secretInput).toHaveValue("new-secret");
+      expect(
+        screen.queryByText(/client secret must be re-entered/i)
+      ).not.toBeInTheDocument();
+    });
+
+    it("keeps the masked client secret when only the client ID is edited", async () => {
+      const { user } = render(<AccountProvisioning {...savedConfigProps} />);
+      await user.type(screen.getByLabelText(/client id/i), "x");
+      expect(screen.getByLabelText(/client secret/i)).toHaveValue("********");
+    });
+
+    it("blocks submission until the client secret is re-entered", async () => {
+      const { user } = render(<AccountProvisioning {...savedConfigProps} />);
+      await user.type(screen.getByLabelText(/token url/i), "x");
+      await user.click(screen.getByRole("button", { name: /save/i }));
+      await waitFor(() => {
+        expect(
+          screen.getByText(/client secret is required/i)
+        ).toBeInTheDocument();
+      });
+      expect(configAPI.update).not.toHaveBeenCalled();
+
+      // the error replaces the "Client secret" label text
+      const secretInput = screen.getByLabelText(/client secret is required/i);
+      await user.type(secretInput, "new-secret");
+      await user.click(screen.getByRole("button", { name: /save/i }));
+
+      await waitFor(() => {
+        expect(configAPI.update).toHaveBeenCalledWith({
+          mdm: {
+            apple_account_provisioning: {
+              oauth_idp_token_url: "https://example.okta.com/oauth2/v1/tokenx",
+              oauth_idp_client_id: "my-client-id",
+              oauth_idp_client_secret: "new-secret",
+            },
+          },
+        });
+      });
+    });
+  });
+
+  describe("Server errors", () => {
+    const fillValidForm = async (user: ReturnType<typeof render>["user"]) => {
+      await user.type(
+        screen.getByLabelText(/token url/i),
+        "https://example.okta.com/oauth2/v1/token"
+      );
+      await user.type(screen.getByLabelText(/client id/i), "my-client-id");
+      await user.type(screen.getByLabelText(/client secret/i), "my-secret");
+      await user.click(screen.getByRole("button", { name: /save/i }));
+    };
+
+    it("surfaces field-level server errors inline on the matching fields and in the toast", async () => {
+      (configAPI.update as jest.Mock).mockRejectedValue({
+        status: 422,
+        data: {
+          message: "Validation Failed",
+          errors: [
+            {
+              name: "mdm.apple_account_provisioning.oauth_idp_client_secret",
+              reason:
+                "oauth_idp_client_secret must be provided when changing oauth_idp_token_url",
+            },
+            {
+              name: "mdm.apple_account_provisioning.oauth_idp_token_url",
+              reason: "must be a valid https URL",
+            },
+          ],
+        },
+      });
+
+      const { user } = render(<AccountProvisioning {...defaultProps} />);
+      await fillValidForm(user);
+
+      await waitFor(() => {
+        expect(
+          screen.getByText(/must be provided when changing/i)
+        ).toBeInTheDocument();
+      });
+      expect(
+        screen.getByText(/must be a valid https url/i)
+      ).toBeInTheDocument();
+      expect(notify.error).toHaveBeenCalledWith(
+        expect.stringContaining("must be provided when changing"),
+        expect.anything()
+      );
+    });
+
+    it("includes the server reason in the error toast for non-field errors", async () => {
+      (configAPI.update as jest.Mock).mockRejectedValue({
+        status: 422,
+        data: {
+          message: "Validation Failed",
+          errors: [
+            {
+              name: "mdm.apple_account_provisioning",
+              reason: "Missing required private key",
+            },
+          ],
+        },
+      });
+
+      const { user } = render(<AccountProvisioning {...defaultProps} />);
+      await fillValidForm(user);
+
+      await waitFor(() => {
+        expect(notify.error).toHaveBeenCalledWith(
+          expect.stringContaining("Missing required private key"),
+          expect.anything()
+        );
      });
    });
  });
@@ -7,6 +7,7 @@ import {
  UNCHANGED_PASSWORD_API_RESPONSE,
 } from "utilities/constants";
 import configAPI from "services/entities/config";
+import { getErrorReason } from "interfaces/errors";
 import { notify } from "components/ToastNotification";
 import { IAppConfigFormProps } from "pages/admin/OrgSettingsPage/cards/constants";

@@ -41,11 +42,9 @@ const validate = (formData: IFormData): IFormErrors => {

  if (!formData.tokenUrl) {
    errors.tokenUrl = "Token URL is required.";
-  } else if (
-    !validUrl({ url: formData.tokenUrl, protocols: ["http", "https"] })
-  ) {
+  } else if (!validUrl({ url: formData.tokenUrl, protocols: ["https"] })) {
    errors.tokenUrl =
-      "Must be a valid URL (e.g. https://yourdomain.okta.com/oauth2/v1/token)";
+      "Must be a valid https URL (e.g. https://yourdomain.okta.com/oauth2/v1/token)";
  }

  if (!formData.clientId) {
@@ -59,6 +58,25 @@ const validate = (formData: IFormData): IFormErrors => {
  return errors;
 };

+const SERVER_ERROR_NAMES: Record<keyof IFormData, string> = {
+  tokenUrl: "mdm.apple_account_provisioning.oauth_idp_token_url",
+  clientId: "mdm.apple_account_provisioning.oauth_idp_client_id",
+  clientSecret: "mdm.apple_account_provisioning.oauth_idp_client_secret",
+};
+
+const getServerFieldErrors = (err: unknown): IFormErrors => {
+  const errors: IFormErrors = {};
+  (Object.keys(SERVER_ERROR_NAMES) as (keyof IFormData)[]).forEach((field) => {
+    const reason = getErrorReason(err, {
+      nameEquals: SERVER_ERROR_NAMES[field],
+    });
+    if (reason) {
+      errors[field] = reason;
+    }
+  });
+  return errors;
+};
+
 const AccountProvisioning = ({ appConfig }: IAppConfigFormProps) => {
  const { gitOpsModeEnabled } = useGitOpsMode();
  const queryClient = useQueryClient();
@@ -83,15 +101,33 @@ const AccountProvisioning = ({ appConfig }: IAppConfigFormProps) => {

  const onInputChange = ({ name, value }: IInputFieldParseTarget) => {
    const newFormData = { ...formData, [name]: value };
-    setFormData(newFormData);
-    // only update errors for fields that already have an error
-    if (formErrors[name as keyof IFormErrors]) {
-      const newErrors = validate(newFormData);
-      setFormErrors((prev) => ({
-        ...prev,
-        [name]: newErrors[name as keyof IFormErrors],
-      }));
+
+    // The server rejects a token URL change that reuses the stored secret
+    // (the secret would be sent to the new, possibly hostile, URL), so clear
+    // the masked secret and have the user re-enter it. Same pattern as
+    // editing a certificate authority.
+    const secretCleared =
+      name === "tokenUrl" &&
+      formData.clientSecret === UNCHANGED_PASSWORD_API_RESPONSE;
+    if (secretCleared) {
+      newFormData.clientSecret = "";
    }
+
+    setFormData(newFormData);
+    setFormErrors((prev) => {
+      const next = { ...prev };
+      if (secretCleared) {
+        next.clientSecret =
+          "Client secret must be re-entered when changing the token URL.";
+      }
+      // only update errors for fields that already have an error
+      if (prev[name as keyof IFormErrors]) {
+        next[name as keyof IFormErrors] = validate(newFormData)[
+          name as keyof IFormErrors
+        ];
+      }
+      return next;
+    });
  };

  const onInputBlur = (field: keyof IFormData) => () => {
@@ -127,8 +163,15 @@ const AccountProvisioning = ({ appConfig }: IAppConfigFormProps) => {
      });
      await queryClient.invalidateQueries(["config"]);
      notify.success("Successfully updated settings.");
-    } catch {
-      notify.error("Failed to update settings.");
+    } catch (err) {
+      setFormErrors((prev) => ({ ...prev, ...getServerFieldErrors(err) }));
+      const reason = getErrorReason(err);
+      notify.error(
+        reason
+          ? `Failed to update settings: ${reason}`
+          : "Failed to update settings.",
+        { response: err }
+      );
    } finally {
      setIsUpdating(false);
    }