Warn & Disallow Creating Role with Existing Name (#132218)

* Adds inline warning (name focus/onBlur) and toast warning (saveRole) when attempting to create a role with a name that already exists. Disallows creating a role with a name that already exists. Event handling efficiency needs review. * Updated API documentation. Implemented unit and functional tests. * Added name compare to throttle GET request from onBlur for efficiency. * Reorganized functional and unit tests. Improved UI logic and presentation. * Update x-pack/plugins/security/server/routes/authorization/roles/put.test.ts Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com> * Update x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.test.tsx Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
elastic · May 25, 2022 · cb403a6 · cb403a6
1 parent 4317eab
commit cb403a6
Show file tree

Hide file tree

Showing 7 changed files with 219 additions and 5 deletions.
diff --git a/docs/api/role-management/put.asciidoc b/docs/api/role-management/put.asciidoc
@@ -51,12 +51,21 @@ To use the create or update role API, you must have the `manage_security` cluste
   To grant access to all spaces, set to `["*"]`, or omit the value.
 =====
 
+[[role-management-api-put-query-params]]
+==== Query parameters
+
+`createOnly`::
+  (Optional, boolean) When `true`, will prevent overwriting the role if it already exists.
+
 [[role-management-api-put-response-codes]]
 ==== Response code
 
 `204`::
   Indicates a successful call.
 
+'409'::
+  When `createOnly` is true, indicates a conflict with an existing role.
+
 ==== Examples
 
 Grant access to various features in all spaces:

diff --git a/x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.test.tsx b/x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.test.tsx
@@ -365,6 +365,102 @@ describe('<EditRolePage />', () => {
     expect(wrapper.find('[data-test-subj="userCannotManageSpacesCallout"]')).toHaveLength(0);
     expectSaveFormButtons(wrapper);
   });
+
+  describe('in create mode', () => {
+    it('renders an error for existing role name', async () => {
+      const props = getProps({ action: 'edit' });
+      const wrapper = mountWithIntl(<EditRolePage {...props} />);
+
+      await waitForRender(wrapper);
+
+      const nameInput = wrapper.find('input[name="name"]');
+      nameInput.simulate('change', { target: { value: 'system_indices_superuser' } });
+      nameInput.simulate('blur');
+
+      await waitForRender(wrapper);
+
+      expect(wrapper.find('EuiFormRow[data-test-subj="roleNameFormRow"]').props()).toMatchObject({
+        error: 'A role with this name already exists.',
+        isInvalid: true,
+      });
+      expectSaveFormButtons(wrapper);
+      expect(wrapper.find('EuiButton[data-test-subj="roleFormSaveButton"]').props().disabled);
+    });
+
+    it('renders an error on save of existing role name', async () => {
+      const props = getProps({ action: 'edit' });
+      const wrapper = mountWithIntl(<EditRolePage {...props} />);
+
+      props.rolesAPIClient.saveRole.mockRejectedValue({
+        body: {
+          statusCode: 409,
+          message: 'Role already exists and cannot be created: system_indices_superuser',
+        },
+      });
+
+      await waitForRender(wrapper);
+
+      const nameInput = wrapper.find('input[name="name"]');
+      const saveButton = wrapper.find('button[data-test-subj="roleFormSaveButton"]');
+
+      nameInput.simulate('change', { target: { value: 'system_indices_superuser' } });
+      saveButton.simulate('click');
+
+      await waitForRender(wrapper);
+
+      expect(wrapper.find('EuiFormRow[data-test-subj="roleNameFormRow"]').props()).toMatchObject({
+        error: 'A role with this name already exists.',
+        isInvalid: true,
+      });
+      // A usual toast notification is not expected with this specific error
+      expect(props.notifications.toasts.addDanger).toBeCalledTimes(0);
+      expectSaveFormButtons(wrapper);
+      expect(wrapper.find('EuiButton[data-test-subj="roleFormSaveButton"]').props().disabled);
+    });
+
+    it('does not render an error for new role name', async () => {
+      const props = getProps({ action: 'edit' });
+      const wrapper = mountWithIntl(<EditRolePage {...props} />);
+
+      props.rolesAPIClient.getRole.mockRejectedValue(new Error('not found'));
+
+      await waitForRender(wrapper);
+
+      const nameInput = wrapper.find('input[name="name"]');
+      nameInput.simulate('change', { target: { value: 'system_indices_superuser' } });
+      nameInput.simulate('blur');
+
+      await waitForRender(wrapper);
+
+      expect(wrapper.find('EuiFormRow[data-test-subj="roleNameFormRow"]').props()).toMatchObject({
+        isInvalid: false,
+      });
+      expectSaveFormButtons(wrapper);
+    });
+
+    it('does not render a notification on save of new role name', async () => {
+      const props = getProps({ action: 'edit' });
+      const wrapper = mountWithIntl(<EditRolePage {...props} />);
+
+      props.rolesAPIClient.getRole.mockRejectedValue(new Error('not found'));
+
+      await waitForRender(wrapper);
+
+      const nameInput = wrapper.find('input[name="name"]');
+      const saveButton = wrapper.find('button[data-test-subj="roleFormSaveButton"]');
+
+      nameInput.simulate('change', { target: { value: 'system_indices_superuser' } });
+      saveButton.simulate('click');
+
+      await waitForRender(wrapper);
+
+      expect(wrapper.find('EuiFormRow[data-test-subj="roleNameFormRow"]').props()).toMatchObject({
+        isInvalid: false,
+      });
+      expect(props.notifications.toasts.addDanger).toBeCalledTimes(0);
+      expectSaveFormButtons(wrapper);
+    });
+  });
 });
 
 async function waitForRender(wrapper: ReactWrapper<any>) {

diff --git a/x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.tsx b/x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.tsx
@@ -19,7 +19,7 @@ import {
   EuiText,
   EuiTitle,
 } from '@elastic/eui';
-import type { ChangeEvent, FunctionComponent, HTMLProps } from 'react';
+import type { ChangeEvent, FocusEvent, FunctionComponent, HTMLProps } from 'react';
 import React, { Fragment, useCallback, useEffect, useRef, useState } from 'react';
 
 import type {
@@ -302,6 +302,8 @@ export const EditRolePage: FunctionComponent<Props> = ({
   // eventually enable validation after the first time user tries to save a role.
   const { current: validator } = useRef(new RoleValidator({ shouldValidate: false }));
   const [formError, setFormError] = useState<RoleValidationResult | null>(null);
+  const [creatingRoleAlreadyExists, setCreatingRoleAlreadyExists] = useState<boolean>(false);
+  const [previousName, setPreviousName] = useState<string>('');
   const runAsUsers = useRunAsUsers(userAPIClient, fatalErrors);
   const indexPatternsTitles = useIndexPatternsTitles(dataViews, fatalErrors, notifications);
   const privileges = usePrivileges(privilegesAPIClient, fatalErrors);
@@ -382,6 +384,7 @@ export const EditRolePage: FunctionComponent<Props> = ({
     return (
       <EuiPanel hasShadow={false} hasBorder={true}>
         <EuiFormRow
+          data-test-subj={'roleNameFormRow'}
           label={
             <FormattedMessage
               id="xpack.security.management.editRole.roleNameFormRowTitle"
@@ -397,13 +400,18 @@ export const EditRolePage: FunctionComponent<Props> = ({
             ) : undefined
           }
           {...validator.validateRoleName(role)}
+          {...(creatingRoleAlreadyExists
+            ? { error: 'A role with this name already exists.', isInvalid: true }
+            : {})}
         >
           <EuiFieldText
             name={'name'}
             value={role.name || ''}
             onChange={onNameChange}
+            onBlur={onNameBlur}
             data-test-subj={'roleFormNameInput'}
             readOnly={isRoleReserved || isEditingExistingRole}
+            isInvalid={creatingRoleAlreadyExists}
           />
         </EuiFormRow>
       </EuiPanel>
@@ -416,6 +424,15 @@ export const EditRolePage: FunctionComponent<Props> = ({
       name: e.target.value,
     });
 
+  const onNameBlur = (e: FocusEvent<HTMLInputElement>) => {
+    if (!isEditingExistingRole && previousName !== role.name) {
+      setPreviousName(role.name);
+      doesRoleExist().then((roleExists) => {
+        setCreatingRoleAlreadyExists(roleExists);
+      });
+    }
+  };
+
   const getElasticsearchPrivileges = () => {
     return (
       <div>
@@ -501,7 +518,7 @@ export const EditRolePage: FunctionComponent<Props> = ({
         data-test-subj={`roleFormSaveButton`}
         fill
         onClick={saveRole}
-        disabled={isRoleReserved}
+        disabled={isRoleReserved || creatingRoleAlreadyExists}
       >
         {saveText}
       </EuiButton>
@@ -529,8 +546,13 @@ export const EditRolePage: FunctionComponent<Props> = ({
       setFormError(null);
 
       try {
-        await rolesAPIClient.saveRole({ role });
+        await rolesAPIClient.saveRole({ role, createOnly: !isEditingExistingRole });
       } catch (error) {
+        if (!isEditingExistingRole && error?.body?.statusCode === 409) {
+          setCreatingRoleAlreadyExists(true);
+          window.scroll({ top: 0, behavior: 'smooth' });
+          return;
+        }
         notifications.toasts.addDanger(
           error?.body?.message ??
             i18n.translate('xpack.security.management.editRole.errorSavingRoleError', {
@@ -551,6 +573,15 @@ export const EditRolePage: FunctionComponent<Props> = ({
     }
   };
 
+  const doesRoleExist = async (): Promise<boolean> => {
+    try {
+      await rolesAPIClient.getRole(role.name);
+      return true;
+    } catch (error) {
+      return false;
+    }
+  };
+
   const handleDeleteRole = async () => {
     try {
       await rolesAPIClient.deleteRole(role.name);

diff --git a/x-pack/plugins/security/public/management/roles/roles_api_client.ts b/x-pack/plugins/security/public/management/roles/roles_api_client.ts
@@ -25,9 +25,10 @@ export class RolesAPIClient {
     await this.http.delete(`/api/security/role/${encodeURIComponent(roleName)}`);
   }
 
-  public async saveRole({ role }: { role: Role }) {
+  public async saveRole({ role, createOnly = false }: { role: Role; createOnly?: boolean }) {
     await this.http.put(`/api/security/role/${encodeURIComponent(role.name)}`, {
       body: JSON.stringify(this.transformRoleForSave(copyRole(role))),
+      query: { createOnly },
     });
   }
 

diff --git a/x-pack/plugins/security/server/routes/authorization/roles/put.test.ts b/x-pack/plugins/security/server/routes/authorization/roles/put.test.ts
@@ -44,6 +44,7 @@ const privilegeMap = {
 
 interface TestOptions {
   name: string;
+  createOnly?: boolean;
   licenseCheckResult?: LicenseCheck;
   apiResponses?: {
     get: () => unknown;
@@ -63,6 +64,7 @@ const putRoleTest = (
   description: string,
   {
     name,
+    createOnly,
     payload,
     licenseCheckResult = { state: 'valid' },
     apiResponses,
@@ -147,6 +149,7 @@ const putRoleTest = (
     const mockRequest = httpServerMock.createKibanaRequest({
       method: 'put',
       path: `/api/security/role/${name}`,
+      query: { createOnly },
       params: { name },
       body: payload !== undefined ? (validate as any).body.validate(payload) : undefined,
       headers,
@@ -271,6 +274,38 @@ describe('PUT role', () => {
         },
       });
     });
+
+    describe('with the create only option enabled', () => {
+      putRoleTest('should fail when role already exists', {
+        name: 'existing-role',
+        createOnly: true,
+        payload: {},
+        apiResponses: {
+          get: () => ({ 'existing-role': 'value-doesnt-matter' }),
+          put: () => {},
+        },
+        asserts: {
+          statusCode: 409,
+          result: {
+            message: 'Role already exists and cannot be created: existing-role',
+          },
+        },
+      });
+
+      putRoleTest(`should succeed when role does not exist`, {
+        name: 'new-role',
+        createOnly: true,
+        payload: {},
+        apiResponses: {
+          get: () => ({}),
+          put: () => {},
+        },
+        asserts: {
+          statusCode: 204,
+          result: undefined,
+        },
+      });
+    });
   });
 
   describe('success', () => {

diff --git a/x-pack/plugins/security/server/routes/authorization/roles/put.ts b/x-pack/plugins/security/server/routes/authorization/roles/put.ts
@@ -47,6 +47,7 @@ export function definePutRolesRoutes({
       path: '/api/security/role/{name}',
       validate: {
         params: schema.object({ name: schema.string({ minLength: 1, maxLength: 1024 }) }),
+        query: schema.object({ createOnly: schema.boolean({ defaultValue: false }) }),
         body: getPutPayloadSchema(() => {
           const privileges = authz.privileges.get();
           return {
@@ -58,7 +59,7 @@ export function definePutRolesRoutes({
     },
     createLicensedRouteHandler(async (context, request, response) => {
       const { name } = request.params;
-
+      const { createOnly } = request.query;
       try {
         const esClient = (await context.core).elasticsearch.client;
         const [features, rawRoles] = await Promise.all([
@@ -77,6 +78,14 @@ export function definePutRolesRoutes({
           });
         }
 
+        if (createOnly && !!rawRoles[name]) {
+          return response.conflict({
+            body: {
+              message: `Role already exists and cannot be created: ${name}`,
+            },
+          });
+        }
+
         const body = transformPutPayloadToElasticsearchRole(
           request.body,
           authz.applicationName,

diff --git a/x-pack/test/api_integration/apis/security/roles.ts b/x-pack/test/api_integration/apis/security/roles.ts
@@ -119,6 +119,38 @@ export default function ({ getService }: FtrProviderContext) {
           })
           .expect(basic ? 403 : 204);
       });
+
+      describe('with the createOnly option enabled', () => {
+        it('should fail when role already exists', async () => {
+          await es.security.putRole({
+            name: 'test_role',
+            body: {
+              cluster: ['monitor'],
+              indices: [
+                {
+                  names: ['beats-*'],
+                  privileges: ['write'],
+                },
+              ],
+              run_as: ['reporting_user'],
+            },
+          });
+
+          await supertest
+            .put('/api/security/role/test_role?createOnly=true')
+            .set('kbn-xsrf', 'xxx')
+            .send({})
+            .expect(409);
+        });
+
+        it('should succeed when role does not exist', async () => {
+          await supertest
+            .put('/api/security/role/new_role?createOnly=true')
+            .set('kbn-xsrf', 'xxx')
+            .send({})
+            .expect(204);
+        });
+      });
     });
 
     describe('Update Role', () => {
@@ -360,6 +392,7 @@ export default function ({ getService }: FtrProviderContext) {
           });
       });
     });
+
     describe('Delete Role', () => {
       it('should delete the roles we created', async () => {
         await supertest.delete('/api/security/role/empty_role').set('kbn-xsrf', 'xxx').expect(204);