Skip to content

Commit

Permalink
[EDR Workflows] Error message 'the value already exists' shown for du…
Browse files Browse the repository at this point in the history 
…plicate values with the 'is one of' operator on Blocklist tab (#196071)

These changes ensure that the warning displayed to the user for
duplicate values remains visible after onBlur. I’ve opted to keep it as
a warning rather than an error since entering a duplicate value doesn’t
allow the user to “save” it as a selected item in the combo box—it
remains an active string in the input field. We don’t block form
submission due to this warning; any unselected value in the combo box is
stripped out. The red border on the combo box is part of EUI’s behavior
when attempting to select a duplicate, and I don’t see an easy way to
modify this at the moment.

https://github.com/user-attachments/assets/84b6d8af-02a8-41f3-88dc-892ed408a098
(cherry picked from commit 99a19e7)
  • Loading branch information
@szwarckonrad
szwarckonrad committed Nov 13, 2024
1 parent 5c55bf3 commit 8cf260e
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 50 deletions.
12 changes: 12 additions & 0 deletions ...curity_solution/public/management/pages/blocklist/view/components/blocklist_form.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,18 @@ describe('blocklist form', () => {
expect(screen.queryByText(ERRORS.INVALID_PATH)).toBeTruthy();
});

it('should prevail duplicate value warning on lost focus', async () => {
const item = createItem({
os_types: [OperatingSystem.WINDOWS],
entries: [createEntry('file.Ext.code_signature', ['valid', 'invalid'])],
});
render(createProps({ item }));
await user.type(screen.getByRole('combobox'), 'invalid{enter}');
expect(screen.queryByText(ERRORS.DUPLICATE_VALUE)).toBeTruthy();
await user.click(screen.getByTestId('blocklist-form-os-select'));
expect(screen.queryByText(ERRORS.DUPLICATE_VALUE)).toBeTruthy();
});

it('should warn if single duplicate value entry', async () => {
const hash = 'C3AB8FF13720E8AD9047DD39466B3C8974E592C2FA383D4A3960714CAEF0C4F2';
const item = createItem({
Expand Down
116 changes: 66 additions & 50 deletions ...ns/security_solution/public/management/pages/blocklist/view/components/blocklist_form.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,8 @@ function isValid(itemValidation: ItemValidation): boolean {
// eslint-disable-next-line react/display-name
export const BlockListForm = memo<ArtifactFormComponentProps>(
({ item, policies, policiesIsLoading, onChange, mode }) => {
const [visited, setVisited] = useState<{ name: boolean; value: boolean }>({
name: false,
value: false,
});
const [nameVisited, setNameVisited] = useState(false);
const [valueVisited, setValueVisited] = useState({ value: false }); // Use object to trigger re-render
const warningsRef = useRef<ItemValidation>({ name: {}, value: {} });
const errorsRef = useRef<ItemValidation>({ name: {}, value: {} });
const [selectedPolicies, setSelectedPolicies] = useState<PolicyData[]>([]);
Expand Down Expand Up @@ -269,57 +267,75 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
);
}, [displaySingleValueInput]);

const validateValues = useCallback((nextItem: ArtifactFormComponentProps['item']) => {
const os = ((nextItem.os_types ?? [])[0] as OperatingSystem) ?? OperatingSystem.WINDOWS;
const {
field = 'file.hash.*',
type = ListOperatorTypeEnum.MATCH_ANY,
value = [],
} = (nextItem.entries[0] ?? {}) as BlocklistEntry;

// value can be a string when isOperator is selected
const values = Array.isArray(value) ? value : [value].filter(Boolean);

const newValueWarnings: ItemValidationNodes = {};
const newNameErrors: ItemValidationNodes = {};
const newValueErrors: ItemValidationNodes = {};
// error if name empty
if (!nextItem.name.trim()) {
newNameErrors.NAME_REQUIRED = createValidationMessage(ERRORS.NAME_REQUIRED);
}
const validateValues = useCallback(
(nextItem: ArtifactFormComponentProps['item'], cleanState = false) => {
const os = ((nextItem.os_types ?? [])[0] as OperatingSystem) ?? OperatingSystem.WINDOWS;
const {
field = 'file.hash.*',
type = ListOperatorTypeEnum.MATCH_ANY,
value = [],
} = (nextItem.entries[0] ?? {}) as BlocklistEntry;

// value can be a string when isOperator is selected
const values = Array.isArray(value) ? value : [value].filter(Boolean);

const newValueWarnings: ItemValidationNodes = cleanState
? {}
: { ...warningsRef.current.value };
const newNameErrors: ItemValidationNodes = cleanState ? {} : { ...errorsRef.current.name };
const newValueErrors: ItemValidationNodes = cleanState
? {}
: { ...errorsRef.current.value };

// error if name empty
if (!nextItem.name.trim()) {
newNameErrors.NAME_REQUIRED = createValidationMessage(ERRORS.NAME_REQUIRED);
} else {
delete newNameErrors.NAME_REQUIRED;
}

// error if no values
if (!values.length) {
newValueErrors.VALUE_REQUIRED = createValidationMessage(ERRORS.VALUE_REQUIRED);
}
// error if no values
if (!values.length) {
newValueErrors.VALUE_REQUIRED = createValidationMessage(ERRORS.VALUE_REQUIRED);
} else {
delete newValueErrors.VALUE_REQUIRED;
}

// error if invalid hash
if (field === 'file.hash.*' && values.some((v) => !isValidHash(v))) {
newValueErrors.INVALID_HASH = createValidationMessage(ERRORS.INVALID_HASH);
}
// error if invalid hash
if (field === 'file.hash.*' && values.some((v) => !isValidHash(v))) {
newValueErrors.INVALID_HASH = createValidationMessage(ERRORS.INVALID_HASH);
} else {
delete newValueErrors.INVALID_HASH;
}

const isInvalidPath = values.some((v) => !isPathValid({ os, field, type, value: v }));
// warn if invalid path
if (field !== 'file.hash.*' && isInvalidPath) {
newValueWarnings.INVALID_PATH = createValidationMessage(ERRORS.INVALID_PATH);
}
// warn if duplicates
if (values.length !== uniq(values).length) {
newValueWarnings.DUPLICATE_VALUES = createValidationMessage(ERRORS.DUPLICATE_VALUES);
}
const isInvalidPath = values.some((v) => !isPathValid({ os, field, type, value: v }));
// warn if invalid path
if (field !== 'file.hash.*' && isInvalidPath) {
newValueWarnings.INVALID_PATH = createValidationMessage(ERRORS.INVALID_PATH);
} else {
delete newValueWarnings.INVALID_PATH;
}
// warn if duplicates
if (values.length !== uniq(values).length) {
newValueWarnings.DUPLICATE_VALUES = createValidationMessage(ERRORS.DUPLICATE_VALUES);
} else {
delete newValueWarnings.DUPLICATE_VALUES;
}

warningsRef.current = { ...warningsRef.current, value: newValueWarnings };
errorsRef.current = { name: newNameErrors, value: newValueErrors };
}, []);
warningsRef.current = { ...warningsRef.current, value: newValueWarnings };
errorsRef.current = { name: newNameErrors, value: newValueErrors };
},
[]
);

const handleOnNameBlur = useCallback(() => {
validateValues(item);
setVisited((prevVisited) => ({ ...prevVisited, name: true }));
setNameVisited(true);
}, [item, validateValues]);

const handleOnValueBlur = useCallback(() => {
validateValues(item);
setVisited((prevVisited) => ({ ...prevVisited, value: true }));
setValueVisited({ value: true });
}, [item, validateValues]);

const handleOnNameChange = useCallback(
Expand Down Expand Up @@ -463,7 +479,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
};

// trigger re-render without modifying item
setVisited((prevVisited) => ({ ...prevVisited }));
setValueVisited((prevState) => ({ ...prevState }));
},
[blocklistEntry]
);
Expand Down Expand Up @@ -495,7 +511,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
entries: [{ ...blocklistEntry, value }],
} as ArtifactFormComponentProps['item'];

validateValues(nextItem);
validateValues(nextItem, true);
onChange({
isValid: isValid(errorsRef.current),
item: nextItem,
Expand All @@ -518,7 +534,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
validateValues(nextItem as ArtifactFormComponentProps['item']);
nextItem.entries[0].value = uniq(nextItem.entries[0].value);

setVisited((prevVisited) => ({ ...prevVisited, value: true }));
setValueVisited({ value: true });
onChange({
isValid: isValid(errorsRef.current),
item: nextItem as ArtifactFormComponentProps['item'],
Expand Down Expand Up @@ -563,7 +579,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(

<EuiFormRow
label={NAME_LABEL}
isInvalid={visited.name && !!Object.keys(errorsRef.current.name).length}
isInvalid={nameVisited && !!Object.keys(errorsRef.current.name).length}
error={Object.values(errorsRef.current.name)}
fullWidth
>
Expand All @@ -572,7 +588,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
value={item.name}
onChange={handleOnNameChange}
onBlur={handleOnNameBlur}
required={visited.name}
required={nameVisited}
maxLength={256}
data-test-subj={getTestId('name-input')}
fullWidth
Expand Down Expand Up @@ -651,7 +667,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
</EuiFormRow>
<EuiFormRow
label={valueLabel}
isInvalid={visited.value && !!Object.keys(errorsRef.current.value).length}
isInvalid={valueVisited.value && !!Object.keys(errorsRef.current.value).length}
helpText={Object.values(warningsRef.current.value)}
error={Object.values(errorsRef.current.value)}
fullWidth
Expand Down

0 comments on commit 8cf260e

Please sign in to comment.