Mark some role editor fields as required (#52477)

gravitational · Feb 27, 2025 · f8c3ccb · f8c3ccb
1 parent 7dba4ae
commit f8c3ccb
Show file tree

Hide file tree

Showing 8 changed files with 38 additions and 32 deletions.
diff --git a/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx b/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx
@@ -87,7 +87,7 @@ test('rendering and switching tabs for new role', async () => {
   expect(
     screen.queryByRole('button', { name: /Reset to Standard Settings/i })
   ).not.toBeInTheDocument();
-  expect(screen.getByLabelText('Role Name')).toHaveValue('new_role_name');
+  expect(screen.getByLabelText('Role Name *')).toHaveValue('new_role_name');
   expect(screen.getByLabelText('Description')).toHaveValue('');
   expect(screen.getByRole('button', { name: 'Create Role' })).toBeEnabled();
 
@@ -109,7 +109,7 @@ test('rendering and switching tabs for new role', async () => {
   expect(screen.getByRole('button', { name: 'Create Role' })).toBeEnabled();
 
   await user.click(getStandardEditorTab());
-  await screen.findByLabelText('Role Name');
+  await screen.findByLabelText('Role Name *');
   expect(
     screen.queryByRole('button', { name: /Reset to Standard Settings/i })
   ).not.toBeInTheDocument();
@@ -138,7 +138,7 @@ test('rendering and switching tabs for a non-standard role', async () => {
 
   await user.click(getStandardEditorTab());
   expect(screen.getByText(/This role is too complex/)).toBeVisible();
-  expect(screen.getByLabelText('Role Name')).toHaveValue('some-role');
+  expect(screen.getByLabelText('Role Name *')).toHaveValue('some-role');
   expect(screen.getByLabelText('Description')).toHaveValue('');
   expect(screen.getByRole('button', { name: 'Save Changes' })).toBeDisabled();
 
@@ -166,10 +166,10 @@ test('switching tabs triggers validation', async () => {
   // Triggering validation is necessary, because server-side yamlification
   // sometimes will reject the data anyway.
   render(<TestRoleEditor />);
-  await user.clear(screen.getByLabelText('Role Name'));
+  await user.clear(screen.getByLabelText('Role Name *'));
   expect(getStandardEditorTab()).toHaveAttribute('aria-selected', 'true');
   await user.click(getYamlEditorTab());
-  expect(screen.getByLabelText('Role Name')).toHaveAccessibleDescription(
+  expect(screen.getByLabelText('Role Name *')).toHaveAccessibleDescription(
     'Role name is required'
   );
   // Expect to still be on the standard tab.
@@ -208,9 +208,9 @@ test('no double conversions when clicking already active tabs', async () => {
   render(<TestRoleEditor />);
   await user.click(getYamlEditorTab());
   await user.click(getStandardEditorTab());
-  await user.type(screen.getByLabelText('Role Name'), '_2');
+  await user.type(screen.getByLabelText('Role Name *'), '_2');
   await user.click(getStandardEditorTab());
-  expect(screen.getByLabelText('Role Name')).toHaveValue('new_role_name_2');
+  expect(screen.getByLabelText('Role Name *')).toHaveValue('new_role_name_2');
 
   await user.click(getYamlEditorTab());
   await user.clear(await findTextEditor());
@@ -317,8 +317,8 @@ test('saving a new role', async () => {
   render(<TestRoleEditor onSave={onSave} />);
   expect(screen.getByRole('button', { name: 'Create Role' })).toBeEnabled();
 
-  await user.clear(screen.getByLabelText('Role Name'));
-  await user.type(screen.getByLabelText('Role Name'), 'great-old-one');
+  await user.clear(screen.getByLabelText('Role Name *'));
+  await user.type(screen.getByLabelText('Role Name *'), 'great-old-one');
   await user.clear(screen.getByLabelText('Description'));
   await user.type(
     screen.getByLabelText('Description'),

diff --git a/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/AccessRules.test.tsx b/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/AccessRules.test.tsx
@@ -50,11 +50,11 @@ describe('AccessRules', () => {
   test('editing', async () => {
     const { user, modelRef } = setup();
     await user.click(screen.getByRole('button', { name: 'Add New' }));
-    await selectEvent.select(screen.getByLabelText('Resources'), [
+    await selectEvent.select(screen.getByLabelText('Resources *'), [
       'db',
       'node',
     ]);
-    await selectEvent.select(screen.getByLabelText('Permissions'), [
+    await selectEvent.select(screen.getByLabelText('Permissions *'), [
       'list',
       'read',
     ]);

diff --git a/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/AccessRules.tsx b/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/AccessRules.tsx
@@ -101,6 +101,7 @@ const AccessRule = memo(function AccessRule({
         components={{ MultiValue: ResourceKindMultiValue }}
         isMulti
         label="Resources"
+        required
         isDisabled={isProcessing}
         options={resourceKindOptions}
         value={resources}
@@ -110,6 +111,7 @@ const AccessRule = memo(function AccessRule({
       <FieldSelect
         isMulti
         label="Permissions"
+        required
         isDisabled={isProcessing}
         options={verbOptions}
         value={verbs}

diff --git a/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/MetadataSection.tsx b/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/MetadataSection.tsx
@@ -42,6 +42,7 @@ export const MetadataSection = ({
   >
     <FieldInput
       label="Role Name"
+      required
       placeholder="Enter Role Name"
       value={value.name}
       disabled={isProcessing}

diff --git a/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/Resources.test.tsx b/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/Resources.test.tsx
@@ -155,13 +155,13 @@ describe('KubernetesAccessSection', () => {
     expect(
       reactSelectValueContainer(screen.getByLabelText('Kind'))
     ).toHaveTextContent('Any kind');
-    expect(screen.getByLabelText('Name')).toHaveValue('*');
-    expect(screen.getByLabelText('Namespace')).toHaveValue('*');
+    expect(screen.getByLabelText('Name *')).toHaveValue('*');
+    expect(screen.getByLabelText('Namespace *')).toHaveValue('*');
     await selectEvent.select(screen.getByLabelText('Kind'), 'Job');
-    await user.clear(screen.getByLabelText('Name'));
-    await user.type(screen.getByLabelText('Name'), 'job-name');
-    await user.clear(screen.getByLabelText('Namespace'));
-    await user.type(screen.getByLabelText('Namespace'), 'job-namespace');
+    await user.clear(screen.getByLabelText('Name *'));
+    await user.type(screen.getByLabelText('Name *'), 'job-name');
+    await user.clear(screen.getByLabelText('Namespace *'));
+    await user.type(screen.getByLabelText('Namespace *'), 'job-namespace');
     await selectEvent.select(screen.getByLabelText('Verbs'), [
       'create',
       'delete',
@@ -200,18 +200,18 @@ describe('KubernetesAccessSection', () => {
     const { user, onChange } = setup();
 
     await user.click(screen.getByRole('button', { name: 'Add a Resource' }));
-    await user.clear(screen.getByLabelText('Name'));
-    await user.type(screen.getByLabelText('Name'), 'res1');
+    await user.clear(screen.getByLabelText('Name *'));
+    await user.type(screen.getByLabelText('Name *'), 'res1');
     await user.click(
       screen.getByRole('button', { name: 'Add Another Resource' })
     );
-    await user.clear(screen.getAllByLabelText('Name')[1]);
-    await user.type(screen.getAllByLabelText('Name')[1], 'res2');
+    await user.clear(screen.getAllByLabelText('Name *')[1]);
+    await user.type(screen.getAllByLabelText('Name *')[1], 'res2');
     await user.click(
       screen.getByRole('button', { name: 'Add Another Resource' })
     );
-    await user.clear(screen.getAllByLabelText('Name')[2]);
-    await user.type(screen.getAllByLabelText('Name')[2], 'res3');
+    await user.clear(screen.getAllByLabelText('Name *')[2]);
+    await user.type(screen.getAllByLabelText('Name *')[2], 'res3');
     expect(onChange).toHaveBeenLastCalledWith(
       expect.objectContaining({
         resources: [
@@ -254,8 +254,8 @@ describe('KubernetesAccessSection', () => {
     await user.click(screen.getByRole('button', { name: 'Add a Label' }));
     await user.click(screen.getByRole('button', { name: 'Add a Resource' }));
     await selectEvent.select(screen.getByLabelText('Kind'), 'Service');
-    await user.clear(screen.getByLabelText('Name'));
-    await user.clear(screen.getByLabelText('Namespace'));
+    await user.clear(screen.getByLabelText('Name *'));
+    await user.clear(screen.getByLabelText('Namespace *'));
     await selectEvent.select(screen.getByLabelText('Verbs'), [
       'All verbs',
       'create',
@@ -267,10 +267,10 @@ describe('KubernetesAccessSection', () => {
     expect(
       screen.getByPlaceholderText('label key')
     ).toHaveAccessibleDescription('required');
-    expect(screen.getByLabelText('Name')).toHaveAccessibleDescription(
+    expect(screen.getByLabelText('Name *')).toHaveAccessibleDescription(
       'Resource name is required, use "*" for any resource'
     );
-    expect(screen.getByLabelText('Namespace')).toHaveAccessibleDescription(
+    expect(screen.getByLabelText('Namespace *')).toHaveAccessibleDescription(
       'Namespace is required for resources of this kind'
     );
     expect(

diff --git a/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/Resources.tsx b/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/Resources.tsx
@@ -57,6 +57,7 @@ import {
   DatabaseAccessValidationResult,
   GitHubOrganizationAccessValidationResult,
   KubernetesAccessValidationResult,
+  kubernetesClusterWideResourceKinds,
   KubernetesResourceValidationResult,
   ResourceAccessValidationResult,
   ServerAccessValidationResult,
@@ -407,6 +408,7 @@ function KubernetesResourceView({
       />
       <FieldInput
         label="Name"
+        required
         toolTipContent={
           <>
             Name of the resource. Special value <MarkInverse>*</MarkInverse>{' '}
@@ -420,6 +422,7 @@ function KubernetesResourceView({
       />
       <FieldInput
         label="Namespace"
+        required={!kubernetesClusterWideResourceKinds.includes(kind.value)}
         toolTipContent={
           <>
             Namespace that contains the resource. Special value{' '}

diff --git a/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/StandardEditor.test.tsx b/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/StandardEditor.test.tsx
@@ -103,15 +103,15 @@ test('collapsed sections still apply validation', async () => {
   const onSave = jest.fn();
   render(<TestStandardEditor onSave={onSave} />);
   // Intentionally cause a validation error.
-  await user.clear(screen.getByLabelText('Role Name'));
+  await user.clear(screen.getByLabelText('Role Name *'));
   // Collapse the section.
   await user.click(screen.getByRole('heading', { name: 'Role Metadata' }));
   await user.click(screen.getByRole('button', { name: 'Create Role' }));
   expect(onSave).not.toHaveBeenCalled();
 
   // Expand the section, make it valid.
   await user.click(screen.getByRole('heading', { name: 'Role Metadata' }));
-  await user.type(screen.getByLabelText('Role Name'), 'foo');
+  await user.type(screen.getByLabelText('Role Name *'), 'foo');
   await user.click(screen.getByRole('button', { name: 'Create Role' }));
   expect(onSave).toHaveBeenCalled();
 });
@@ -121,15 +121,15 @@ test('invisible tabs still apply validation', async () => {
   const onSave = jest.fn();
   render(<TestStandardEditor onSave={onSave} />);
   // Intentionally cause a validation error.
-  await user.clear(screen.getByLabelText('Role Name'));
+  await user.clear(screen.getByLabelText('Role Name *'));
   // Switch to a different tab.
   await user.click(screen.getByRole('tab', { name: 'Resources' }));
   await user.click(screen.getByRole('button', { name: 'Create Role' }));
   expect(onSave).not.toHaveBeenCalled();
 
   // Switch back, make it valid.
   await user.click(screen.getByRole('tab', { name: 'Invalid data Overview' }));
-  await user.type(screen.getByLabelText('Role Name'), 'foo');
+  await user.type(screen.getByLabelText('Role Name *'), 'foo');
   await user.click(screen.getByRole('button', { name: 'Create Role' }));
   expect(onSave).toHaveBeenCalled();
 });

diff --git a/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/validation.ts b/web/packages/teleport/src/Roles/RoleEditor/StandardEditor/validation.ts
@@ -40,7 +40,7 @@ import {
   RuleModel,
 } from './standardmodel';
 
-const kubernetesClusterWideResourceKinds: KubernetesResourceKind[] = [
+export const kubernetesClusterWideResourceKinds: KubernetesResourceKind[] = [
   'namespace',
   'kube_node',
   'persistentvolume',