Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Load FLS suggestions on-demand #98681

Merged
merged 10 commits into from May 5, 2021
Merged

Load FLS suggestions on-demand #98681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
52281ef
Add filter_path option to reduce size of response payload
legrego Apr 28, 2021
428d0b8
Only load FLS suggestions on-demand
legrego Apr 28, 2021
d0aca90
Additional testing
legrego Apr 29, 2021
8f1ae02
Add API integration test for 404 condition
legrego Apr 29, 2021
3fccd1e
Merge branch 'master' into legrego/issue47378
kibanamachine May 3, 2021
1f49b6e
Merge branch 'master' into legrego/issue47378
kibanamachine May 3, 2021
9af7274
Merge branch 'master' of github.com:elastic/kibana into legrego/issue…
legrego May 4, 2021
99fd55f
Address PR feedback
legrego May 4, 2021
8886467
Merge branch 'legrego/issue47378' of github.com:legrego/kibana into l…
legrego May 4, 2021
ee310d3
Fix unit test
legrego May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
130 changes: 120 additions & 10 deletions ...ns/security/public/management/roles/edit_role/privileges/es/index_privilege_form.test.tsx
View file
Open in desktop
Expand Up @@ -5,11 +5,12 @@
* 2.0.
*/

import { EuiButtonIcon, EuiTextArea } from '@elastic/eui';
import { EuiButtonIcon, EuiComboBox, EuiTextArea } from '@elastic/eui';
import React from 'react';

import { mountWithIntl, shallowWithIntl } from '@kbn/test/jest';
import { findTestSubject, mountWithIntl, nextTick, shallowWithIntl } from '@kbn/test/jest';

import { indicesAPIClientMock } from '../../../index.mock';
import { RoleValidator } from '../../validate_role';
import { IndexPrivilegeForm } from './index_privilege_form';

Expand All @@ -25,7 +26,7 @@ test('it renders without crashing', () => {
},
formIndex: 0,
indexPatterns: [],
availableFields: [],
indicesAPIClient: indicesAPIClientMock.create(),
availableIndexPrivileges: ['all', 'read', 'write', 'index'],
isRoleReadOnly: false,
allowDocumentLevelSecurity: true,
Expand All @@ -52,7 +53,7 @@ test('it allows for custom index privileges', () => {
},
formIndex: 0,
indexPatterns: [],
availableFields: [],
indicesAPIClient: indicesAPIClientMock.create(),
availableIndexPrivileges: ['all', 'read', 'write', 'index'],
isRoleReadOnly: false,
allowDocumentLevelSecurity: true,
Expand Down Expand Up @@ -86,7 +87,7 @@ describe('delete button', () => {
},
formIndex: 0,
indexPatterns: [],
availableFields: [],
indicesAPIClient: indicesAPIClientMock.create(),
availableIndexPrivileges: ['all', 'read', 'write', 'index'],
isRoleReadOnly: false,
allowDocumentLevelSecurity: true,
Expand Down Expand Up @@ -138,7 +139,7 @@ describe(`document level security`, () => {
},
formIndex: 0,
indexPatterns: [],
availableFields: [],
indicesAPIClient: indicesAPIClientMock.create(),
availableIndexPrivileges: ['all', 'read', 'write', 'index'],
isRoleReadOnly: false,
allowDocumentLevelSecurity: true,
Expand Down Expand Up @@ -197,7 +198,7 @@ describe('field level security', () => {
},
formIndex: 0,
indexPatterns: [],
availableFields: [],
indicesAPIClient: indicesAPIClientMock.create(),
availableIndexPrivileges: ['all', 'read', 'write', 'index'],
isRoleReadOnly: false,
allowDocumentLevelSecurity: true,
Expand All @@ -208,19 +209,21 @@ describe('field level security', () => {
intl: {} as any,
};

test(`inputs are hidden when FLS is not allowed`, () => {
test(`inputs are hidden when FLS is not allowed, and fields are not queried`, async () => {
const testProps = {
...props,
allowFieldLevelSecurity: false,
};

const wrapper = mountWithIntl(<IndexPrivilegeForm {...testProps} />);
await nextTick();
expect(wrapper.find('EuiSwitch[data-test-subj="restrictFieldsQuery0"]')).toHaveLength(0);
expect(wrapper.find('.indexPrivilegeForm__grantedFieldsRow')).toHaveLength(0);
expect(wrapper.find('.indexPrivilegeForm__deniedFieldsRow')).toHaveLength(0);
expect(testProps.indicesAPIClient.getFields).not.toHaveBeenCalled();
});

test('only the switch is shown when allowed, and FLS is empty', () => {
test('renders the FLS switch when available, but collapsed when no fields are selected', async () => {
const testProps = {
...props,
indexPrivilege: {
Expand All @@ -230,19 +233,126 @@ describe('field level security', () => {
};

const wrapper = mountWithIntl(<IndexPrivilegeForm {...testProps} />);
await nextTick();
expect(wrapper.find('EuiSwitch[data-test-subj="restrictFieldsQuery0"]')).toHaveLength(1);
expect(wrapper.find('.indexPrivilegeForm__grantedFieldsRow')).toHaveLength(0);
expect(wrapper.find('.indexPrivilegeForm__deniedFieldsRow')).toHaveLength(0);
expect(testProps.indicesAPIClient.getFields).not.toHaveBeenCalled();
});

test('inputs are shown when allowed', () => {
test('FLS inputs are shown when allowed', async () => {
const testProps = {
...props,
};

const wrapper = mountWithIntl(<IndexPrivilegeForm {...testProps} />);
await nextTick();
expect(wrapper.find('div.indexPrivilegeForm__grantedFieldsRow')).toHaveLength(1);
expect(wrapper.find('div.indexPrivilegeForm__deniedFieldsRow')).toHaveLength(1);
expect(testProps.indicesAPIClient.getFields).not.toHaveBeenCalled();
});

test('does not query for available fields when a request is already in flight', async () => {
jest.useFakeTimers();

const testProps = {
...props,
indexPrivilege: {
...props.indexPrivilege,
names: ['foo', 'bar-*'],
},
indicesAPIClient: indicesAPIClientMock.create(),
};

testProps.indicesAPIClient.getFields.mockImplementation(async () => {
return new Promise((resolve) =>
setTimeout(() => {
resolve(['foo']);
}, 5000)
);
});

const wrapper = mountWithIntl(<IndexPrivilegeForm {...testProps} />);
await nextTick();
expect(wrapper.find('div.indexPrivilegeForm__grantedFieldsRow')).toHaveLength(1);
expect(wrapper.find('div.indexPrivilegeForm__deniedFieldsRow')).toHaveLength(1);
expect(testProps.indicesAPIClient.getFields).toHaveBeenCalledTimes(1);

findTestSubject(wrapper, 'fieldInput0').simulate('focus');
jest.advanceTimersByTime(2000);
expect(testProps.indicesAPIClient.getFields).toHaveBeenCalledTimes(1);

findTestSubject(wrapper, 'fieldInput0').simulate('focus');
jest.advanceTimersByTime(4000);
expect(testProps.indicesAPIClient.getFields).toHaveBeenCalledTimes(1);
});

test('queries for available fields when mounted, and FLS is available', async () => {
const testProps = {
...props,
indexPrivilege: {
...props.indexPrivilege,
names: ['foo', 'bar-*'],
},
indicesAPIClient: indicesAPIClientMock.create(),
};

testProps.indicesAPIClient.getFields.mockResolvedValue(['a', 'b', 'c']);

const wrapper = mountWithIntl(<IndexPrivilegeForm {...testProps} />);
await nextTick();
expect(wrapper.find('div.indexPrivilegeForm__grantedFieldsRow')).toHaveLength(1);
expect(wrapper.find('div.indexPrivilegeForm__deniedFieldsRow')).toHaveLength(1);
expect(testProps.indicesAPIClient.getFields).toHaveBeenCalledTimes(1);
});

test('does not query for available fields when mounted, and FLS is unavailable', async () => {
const testProps = {
...props,
indexPrivilege: {
...props.indexPrivilege,
names: ['foo', 'bar-*'],
},
indicesAPIClient: indicesAPIClientMock.create(),
allowFieldLevelSecurity: false,
};

testProps.indicesAPIClient.getFields.mockResolvedValue(['a', 'b', 'c']);

const wrapper = mountWithIntl(<IndexPrivilegeForm {...testProps} />);
await nextTick();
expect(wrapper.find('div.indexPrivilegeForm__grantedFieldsRow')).toHaveLength(0);
expect(wrapper.find('div.indexPrivilegeForm__deniedFieldsRow')).toHaveLength(0);
expect(testProps.indicesAPIClient.getFields).not.toHaveBeenCalled();
});

test('queries for available fields when the set of index patterns change', async () => {
const testProps = {
...props,
indexPrivilege: {
...props.indexPrivilege,
names: ['foo', 'bar-*'],
},
indexPatterns: ['foo', 'bar-*', 'newPattern'],
indicesAPIClient: indicesAPIClientMock.create(),
};

testProps.indicesAPIClient.getFields.mockResolvedValue(['a', 'b', 'c']);

const wrapper = mountWithIntl(<IndexPrivilegeForm {...testProps} />);
await nextTick();
expect(wrapper.find('div.indexPrivilegeForm__grantedFieldsRow')).toHaveLength(1);
expect(wrapper.find('div.indexPrivilegeForm__deniedFieldsRow')).toHaveLength(1);
expect(testProps.indicesAPIClient.getFields).toHaveBeenCalledTimes(1);

wrapper
.find(EuiComboBox)
.filterWhere((item) => item.props()['data-test-subj'] === 'indicesInput0')
.props().onChange!([{ label: 'newPattern', value: 'newPattern' }]);

await nextTick();
expect(testProps.indicesAPIClient.getFields).toHaveBeenCalledTimes(2);
expect(testProps.indicesAPIClient.getFields).toHaveBeenCalledWith('newPattern');
});

test('it displays a warning when no fields are granted', () => {
Expand Down
64 changes: 59 additions & 5 deletions ...plugins/security/public/management/roles/edit_role/privileges/es/index_privilege_form.tsx
View file
Open in desktop
Expand Up @@ -23,8 +23,10 @@ import React, { Component, Fragment } from 'react';

import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import type { PublicMethodsOf } from '@kbn/utility-types';

import type { RoleIndexPrivilege } from '../../../../../../common/model';
import type { IndicesAPIClient } from '../../../indices_api_client';
import type { RoleValidator } from '../../validate_role';

const fromOption = (option: any) => option.label;
Expand All @@ -35,7 +37,7 @@ interface Props {
indexPrivilege: RoleIndexPrivilege;
indexPatterns: string[];
availableIndexPrivileges: string[];
availableFields: string[];
indicesAPIClient: PublicMethodsOf<IndicesAPIClient>;
onChange: (indexPrivilege: RoleIndexPrivilege) => void;
onDelete: () => void;
isRoleReadOnly: boolean;
Expand All @@ -50,9 +52,16 @@ interface State {
grantedFields: string[];
exceptedFields: string[];
documentQuery?: string;
isFieldListLoading: boolean;
flsOptions: string[];
}

export class IndexPrivilegeForm extends Component<Props, State> {
// This is distinct from the field within `this.state`.
// We want to make sure that only one request for fields is in-flight at a time,
// and relying on state for this is error prone.
private isFieldListLoading: boolean = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: just for my own education, would you mind explaining a bit when storing this flag only is state leads to errors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React can decide to batch/defer state updates, so calling this.setState() doesn't guarantee that your state will be updated within a specific period of time. Having this private field outside of state works around this by giving us a synchronously updating field that always reflects reality.


constructor(props: Props) {
super(props);

Expand All @@ -64,9 +73,17 @@ export class IndexPrivilegeForm extends Component<Props, State> {
grantedFields: grant,
exceptedFields: except,
documentQuery: props.indexPrivilege.query,
isFieldListLoading: false,
flsOptions: [],
};
}

public componentDidMount() {
if (this.state.fieldSecurityExpanded && this.props.allowFieldLevelSecurity) {
this.loadFLSOptions(this.props.indexPrivilege.names);
}
}

public render() {
return (
<Fragment>
Expand Down Expand Up @@ -149,11 +166,30 @@ export class IndexPrivilegeForm extends Component<Props, State> {
);
};

private loadFLSOptions = (indexNames: string[], force = false) => {
if (!force && (this.isFieldListLoading || indexNames.length === 0)) return;

this.isFieldListLoading = true;
this.setState({
isFieldListLoading: true,
});
watson marked this conversation as resolved.
Show resolved Hide resolved

this.props.indicesAPIClient
.getFields(indexNames.join(','))
.then((fields) => {
this.isFieldListLoading = false;
this.setState({ flsOptions: fields, isFieldListLoading: false });
})
.catch(() => {
this.isFieldListLoading = false;
this.setState({ flsOptions: [], isFieldListLoading: false });
});
};

private getFieldLevelControls = () => {
const {
allowFieldLevelSecurity,
allowDocumentLevelSecurity,
availableFields,
indexPrivilege,
isRoleReadOnly,
} = this.props;
Expand Down Expand Up @@ -210,11 +246,13 @@ export class IndexPrivilegeForm extends Component<Props, State> {
<Fragment>
<EuiComboBox
data-test-subj={`fieldInput${this.props.formIndex}`}
options={availableFields ? availableFields.map(toOption) : []}
options={this.state.flsOptions.map(toOption)}
selectedOptions={grant.map(toOption)}
onCreateOption={this.onCreateGrantedField}
onChange={this.onGrantedFieldsChange}
isDisabled={this.props.isRoleReadOnly}
async={true}
isLoading={this.state.isFieldListLoading}
/>
</Fragment>
</EuiFormRow>
Expand All @@ -233,11 +271,13 @@ export class IndexPrivilegeForm extends Component<Props, State> {
<Fragment>
<EuiComboBox
data-test-subj={`deniedFieldInput${this.props.formIndex}`}
options={availableFields ? availableFields.map(toOption) : []}
options={this.state.flsOptions.map(toOption)}
selectedOptions={except.map(toOption)}
onCreateOption={this.onCreateDeniedField}
onChange={this.onDeniedFieldsChange}
isDisabled={isRoleReadOnly}
async={true}
isLoading={this.state.isFieldListLoading}
/>
</Fragment>
</EuiFormRow>
Expand Down Expand Up @@ -362,6 +402,11 @@ export class IndexPrivilegeForm extends Component<Props, State> {
const hasSavedFieldSecurity =
this.state.exceptedFields.length > 0 || this.state.grantedFields.length > 0;

// If turning on, then request available fields
if (willToggleOn) {
this.loadFLSOptions(this.props.indexPrivilege.names);
}

if (willToggleOn && !hasConfiguredFieldSecurity && hasSavedFieldSecurity) {
this.props.onChange({
...this.props.indexPrivilege,
Expand All @@ -380,13 +425,22 @@ export class IndexPrivilegeForm extends Component<Props, State> {
...this.props.indexPrivilege,
names: newIndexPatterns,
});
// If FLS controls are visible, then forcefully request a new set of options
if (this.state.fieldSecurityExpanded) {
this.loadFLSOptions(newIndexPatterns, true);
}
};

private onIndexPatternsChange = (newPatterns: EuiComboBoxOptionOption[]) => {
const names = newPatterns.map(fromOption);
this.props.onChange({
...this.props.indexPrivilege,
names: newPatterns.map(fromOption),
names,
});
// If FLS controls are visible, then forcefully request a new set of options
if (this.state.fieldSecurityExpanded) {
this.loadFLSOptions(names, true);
}
};

private onPrivilegeChange = (newPrivileges: EuiComboBoxOptionOption[]) => {
Expand Down
5 changes: 4 additions & 1 deletion ...lugins/security/public/management/roles/edit_role/privileges/es/index_privileges.test.tsx
View file
Open in desktop
Expand Up @@ -56,6 +56,9 @@ test('it renders a IndexPrivilegeForm for each privilege on the role', async ()
allowRoleDocumentLevelSecurity: true,
} as any);

const indicesAPIClient = indicesAPIClientMock.create();
indicesAPIClient.getFields.mockResolvedValue(['foo']);

const props = {
role: {
name: '',
Expand All @@ -80,7 +83,7 @@ test('it renders a IndexPrivilegeForm for each privilege on the role', async ()
editable: true,
validator: new RoleValidator(),
availableIndexPrivileges: ['all', 'read', 'write', 'index'],
indicesAPIClient: indicesAPIClientMock.create(),
indicesAPIClient,
license,
};
const wrapper = mountWithIntl(<IndexPrivileges {...props} />);
Expand Down