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>
This commit is contained in:
Jeramy Soucy 2022-05-25 17:34:41 +02:00 committed by GitHub
parent 4317eabbea
commit cb403a6fa0
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 219 additions and 5 deletions

View file

@ -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:

View file

@ -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>) {

View file

@ -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);

View file

@ -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 },
});
}

View file

@ -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', () => {

View file

@ -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,

View file

@ -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);