Conditionally render silo creation fields, depending on identity mode

[charliepark]

Fixes #2325

The issue above noted that when creating a "local only" silo (API docs here), the admin group name wasn't relevant, so we should only show it during silo creation when the identity mode is saml_jit. Looking into Omicron a bit, it turns out that the admin group name is, in turn, necessary in order to assign fleet roles. Here's that code:
https://github.com/oxidecomputer/omicron/blob/89b998c1c24e26b9e1161acbbd1b236f40470bdd/nexus/db-queries/src/db/datastore/silo.rs#L205-L227

This PR sets the silo creation form to conditionally render the admin group name and the "assign fleet role" checkboxes. It also adds logic to make sure that if the name is deleted that the checkboxes are cleared, and that if the identity mode is switched to "local", that the admin group name is cleared.

Some screenshots, to illustrate the above:

1. Here's the form in its natural state:

2. We've entered some data:

3. Oh, wait, let's make this one local only:

4. And a quick check to verify that the fields cleared:

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Jul 24, 2024 8:21pm

[david-crespo]

I tried doing this stuff in onChange on identity mode and admin group name instead, which is vaguely to be preferred over useEffect when you can make it work, because cuts out a render or two that have to happen while each of the state values update in turn and the useEffects fire. But it didn't really work out.

The result was a bit sad in that a) I lost the ability to clear the checkboxes when the group name clears (because setValue doesn't trigger onChange), and b) it requires a change to how we handle onChange in the text field and radio field that I am very unsure about. I had to make so when you pass in onChange, it runs in addition to the field one instead of overriding it. There's a good chance we are relying on that overriding behavior, plus it would be weird to have some fields work this way and others not. I'm sure the watch and useEffect version won't cause perf problems anyway, plus it's clearer because the related logic is all together.

failed experiment diff

diff --git a/app/components/form/fields/RadioField.tsx b/app/components/form/fields/RadioField.tsx
index 1fdc9c50..f2f7ece3 100644
--- a/app/components/form/fields/RadioField.tsx
+++ b/app/components/form/fields/RadioField.tsx
@@ -71,6 +71,7 @@ export function RadioField<
   control,
   items,
   parseValue,
+  onChange,
   ...props
 }: RadioFieldProps<TFieldValues, TName>) {
   const id = useId()
@@ -92,9 +93,11 @@ export function RadioField<
           [`${id}-help-text`]: !!tooltipText,
         })}
         aria-describedby={tooltipText ? `${id}-label-tip` : undefined}
-        onChange={(e) =>
-          parseValue ? field.onChange(parseValue(e.target.value)) : field.onChange(e)
-        }
+        onChange={(e) => {
+          // if onChange is passed in, call it in addition to field one, not instead of it
+          onChange?.(e)
+          return parseValue ? field.onChange(parseValue(e.target.value)) : field.onChange(e)
+        }}
         name={field.name}
         {...props}
         // TODO: once we get rid of the other use of RadioGroup, change RadioGroup
diff --git a/app/components/form/fields/TextField.tsx b/app/components/form/fields/TextField.tsx
index 6662fa07..5dff4aac 100644
--- a/app/components/form/fields/TextField.tsx
+++ b/app/components/form/fields/TextField.tsx
@@ -114,12 +114,13 @@ export const TextFieldInner = <
   required,
   id: idProp,
   transform,
+  onChange,
   ...props
 }: TextFieldProps<TFieldValues, TName> & UITextAreaProps) => {
   const generatedId = useId()
   const id = idProp || generatedId
   const {
-    field: { onChange, ...fieldRest },
+    field: { onChange: fieldOnChange, ...fieldRest },
     fieldState: { error },
   } = useController({ name, control, rules: { required, validate } })
   return (
@@ -131,7 +132,10 @@ export const TextFieldInner = <
         error={!!error}
         aria-labelledby={cn(`${id}-label`, !!tooltipText && `${id}-help-text`)}
         aria-describedby={tooltipText ? `${id}-label-tip` : undefined}
-        onChange={(e) => onChange(transform ? transform(e.target.value) : e.target.value)}
+        onChange={(e) => {
+          onChange?.(e)
+          return fieldOnChange(transform ? transform(e.target.value) : e.target.value)
+        }}
         {...fieldRest}
         {...props}
       />
diff --git a/app/forms/silo-create.tsx b/app/forms/silo-create.tsx
index a677b2a7..5b73cae0 100644
--- a/app/forms/silo-create.tsx
+++ b/app/forms/silo-create.tsx
@@ -68,20 +68,8 @@ export function CreateSiloSideModalForm() {
   const form = useForm({ defaultValues })
   const identityMode = form.watch('identityMode')
   const adminGroupName = form.watch('adminGroupName')
-  // Clear the adminGroupName if the user selects the "local only" identity mode
-  useEffect(() => {
-    if (identityMode === 'local_only') {
-      form.setValue('adminGroupName', '')
-    }
-  }, [identityMode, form])
-  // Clear the role assignment checkboxes if the adminGroupName is deleted
   const fleetRolesDisabled = !adminGroupName
-  useEffect(() => {
-    if (fleetRolesDisabled) {
-      form.setValue('siloAdminGetsFleetAdmin', false)
-      form.setValue('siloViewerGetsFleetViewer', false)
-    }
-  }, [fleetRolesDisabled, form])
+
   return (
     <SideModalForm
       form={form}
@@ -160,6 +148,14 @@ export function CreateSiloSideModalForm() {
           { value: 'saml_jit', label: 'SAML' },
           { value: 'local_only', label: 'Local only' },
         ]}
+        onChange={(e) => {
+          // Clear SAML-specific fields when user selects "local only"
+          if (e.target.value === 'local_only') {
+            form.setValue('adminGroupName', '')
+            form.setValue('siloAdminGetsFleetAdmin', false)
+            form.setValue('siloViewerGetsFleetViewer', false)
+          }
+        }}
       />
       {identityMode === 'saml_jit' && (
         <>

[david-crespo]

TL;DR: only the admin group name is conditional. Checkboxes are always relevant.

---

After taking another look at the backend code where we determined that we should hide the fleet role checkboxes if the user is making a local silo, I think we read it wrong. That code only concerns what is done with the admin group name. There is a reference to policy.role_assignments which I think we misread as concerning the checkbox values, but those are in mapped_fleet_roles, and that gets passed straight through to the DB record for the silo regardless of the presence of the admin group name.

This makes sense, because the fleet role mapping doesn't have anything to do with the admin group per se. It just says: if a user is an admin/viewer in this silo, make them an admin/viewer on the fleet. Where the user got that admin role from, whether, e.g., by direct assignment or by being in a designated admin group, is irrelevant. The Polar code uses a custom Rust function to look at the actor's roles on the silo and the fleet role mapping and decides whether they also have a role on the fleet.

[charliepark]

After a few tweaks to general layout: