Error on firewall rule name conflicts (#2358)

* error on firewall rule name conflicts

* do it the right way, hooking into the validate prop on NameField

* on rule update success, do onDismiss nav before invalidating rules view query

Author: david-crespo

app/components/form/fields/NameField.tsx

@@ -18,11 +18,15 @@ export function NameField<
   required = true,
   name,
   label = capitalize(name),
+  validate,
   ...textFieldProps
-}: Omit<TextFieldProps<TFieldValues, TName>, 'validate'> & { label?: string }) {
+}: TextFieldProps<TFieldValues, TName> & { label?: string }) {
   return (
     <TextField
-      validate={(name) => validateName(name, label, required)}
+      // always check the name rules first, then the other checks if present
+      validate={(name, formValues) =>
+        validateName(name, label, required) || validate?.(name, formValues)
+      }
       required={required}
       label={label}
       name={name}

app/forms/firewall-rules-create.tsx

@@ -104,6 +104,7 @@ const targetDefaultValues: TargetFormValues = {
 type CommonFieldsProps = {
   error: ApiError | null
   control: Control<FirewallRuleValues>
+  nameTaken: (name: string) => boolean
 }
 
 function getFilterValueProps(hostType: VpcFirewallRuleHostFilter['type']) {
@@ -155,7 +156,7 @@ const DocsLinkMessage = () => (
   />
 )
 
-export const CommonFields = ({ error, control }: CommonFieldsProps) => {
+export const CommonFields = ({ error, control, nameTaken }: CommonFieldsProps) => {
   const portRangeForm = useForm({ defaultValues: portRangeDefaultValues })
   const ports = useController({ name: 'ports', control }).field
   const submitPortRange = portRangeForm.handleSubmit(({ portRange }) => {
@@ -199,7 +200,16 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => {
       <CheckboxField name="enabled" control={control}>
         Enabled
       </CheckboxField>
-      <NameField name="name" control={control} />
+      <NameField
+        name="name"
+        control={control}
+        validate={(name) => {
+          if (nameTaken(name)) {
+            // TODO: might be worth mentioning that the names are unique per VPC as opposed to globally
+            return 'Name taken. To update an existing rule, edit it directly.'
+          }
+        }}
+      />
       <DescriptionField name="description" control={control} />
 
       <RadioField
@@ -589,23 +599,29 @@ export function CreateFirewallRuleForm() {
       title="Add firewall rule"
       onDismiss={onDismiss}
       onSubmit={(values) => {
-        // TODO: this silently overwrites existing rules with the current name.
-        // we should probably at least warn and confirm, if not reject as invalid
-        const otherRules = existingRules
-          .filter((r) => r.name !== values.name)
-          .map(firewallRuleGetToPut)
         updateRules.mutate({
           query: vpcSelector,
           body: {
-            rules: [...otherRules, valuesToRuleUpdate(values)],
+            rules: [...existingRules.map(firewallRuleGetToPut), valuesToRuleUpdate(values)],
           },
         })
       }}
       loading={updateRules.isPending}
       submitError={updateRules.error}
       submitLabel="Add rule"
     >
-      <CommonFields error={updateRules.error} control={form.control} />
+      <CommonFields
+        error={updateRules.error}
+        control={form.control}
+        // error if name is already in use
+        nameTaken={(name) => !!data.rules.find((r) => r.name === name)}
+
+        // TODO: there should also be a form-level error so if the name is off
+        // screen, it doesn't look like the submit button isn't working. Maybe
+        // instead of setting a root error, it would be more robust to show a
+        // top level error if any errors are found in the form. We might want to
+        // expand the submitError prop on SideModalForm for this
+      />
     </SideModalForm>
   )
 }

app/forms/firewall-rules-edit.tsx

@@ -61,8 +61,12 @@ export function EditFirewallRuleForm() {
 
   const updateRules = useApiMutation('vpcFirewallRulesUpdate', {
     onSuccess() {
-      queryClient.invalidateQueries('vpcFirewallRulesView')
+      // Nav before the invalidate because I once saw the above invariant fail
+      // briefly after successful edit (error page flashed but then we land
+      // on the rules list ok) and I think it was a race condition where the
+      // invalidate managed to complete while the modal was still open.
       onDismiss()
+      queryClient.invalidateQueries('vpcFirewallRulesView')
     },
   })
 
@@ -87,32 +91,35 @@ export function EditFirewallRuleForm() {
   // TODO: uhhhh how can this happen
   if (Object.keys(originalRule).length === 0) return null
 
+  // note different filter logic from create: filter out the rule with the
+  // *original* name because we need to overwrite that rule
+  const otherRules = data.rules.filter((r) => r.name !== originalRule.name)
+
   return (
     <SideModalForm
       form={form}
       formType="edit"
       resourceName="rule"
       onDismiss={onDismiss}
-      onSubmit={(values) => {
-        // note different filter logic from create: filter out the rule with the
-        // *original* name because we need to overwrite that rule
-        const otherRules = data.rules
-          .filter((r) => r.name !== originalRule.name)
-          .map(firewallRuleGetToPut)
-
+      onSubmit={(values) =>
         updateRules.mutate({
           query: vpcSelector,
           body: {
-            rules: [...otherRules, valuesToRuleUpdate(values)],
+            rules: [...otherRules.map(firewallRuleGetToPut), valuesToRuleUpdate(values)],
           },
         })
-      }}
+      }
       // validationSchema={validationSchema}
       // validateOnBlur
       loading={updateRules.isPending}
       submitError={updateRules.error}
     >
-      <CommonFields error={updateRules.error} control={form.control} />
+      <CommonFields
+        error={updateRules.error}
+        control={form.control}
+        // error if name is being changed to something that conflicts with some other rule
+        nameTaken={(name) => !!otherRules.find((r) => r.name === name)}
+      />
     </SideModalForm>
   )
 }

test/e2e/firewall-rules.e2e.ts

@@ -371,3 +371,48 @@ test('404s on edit non-existent rule', async ({ page }) => {
   await page.goto(rulePath.replace('icmp', 'boop'))
   await expect(page.getByText('Page not found')).toBeVisible()
 })
+
+// when creating a rule, giving it the same name as an existing rule is an
+// error. if you want to overwrite a rule, you need to edit it
+test('name conflict error on create', async ({ page }) => {
+  await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new')
+
+  await page.getByRole('textbox', { name: 'Name', exact: true }).fill('allow-ssh')
+
+  const error = page.getByText('Name taken').first()
+  await expect(error).toBeHidden()
+
+  await page.getByRole('button', { name: 'Add rule' }).click()
+  await expect(error).toBeVisible()
+})
+
+// when editing a rule, on the other hand, we only check for conflicts against rules
+// other than the one being edited, because of course its name can stay the same
+test('name conflict error on edit', async ({ page }) => {
+  await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules/allow-icmp/edit')
+
+  // changing the name to one taken by another rule is an error
+  const nameField = page.getByRole('textbox', { name: 'Name', exact: true })
+  await nameField.fill('allow-ssh')
+
+  const error = page.getByText('Name taken').first()
+  await expect(error).toBeHidden()
+
+  await page.getByRole('button', { name: 'Update rule' }).click()
+  await expect(error).toBeVisible()
+
+  // change name back
+  await nameField.fill('allow-icmp')
+
+  // changing a value _without_ changing the name is allowed
+  await page.getByRole('textbox', { name: 'Priority' }).fill('37')
+  await page.getByRole('button', { name: 'Update rule' }).click()
+  await expect(error).toBeHidden()
+  await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp', Priority: '37' })
+
+  // changing the name to a non-conflicting name is allowed
+  await page.getByRole('link', { name: 'allow-icmp' }).click()
+  await nameField.fill('allow-icmp2')
+  await page.getByRole('button', { name: 'Update rule' }).click()
+  await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp2', Priority: '37' })
+})