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

[Security solution][Endpoint] Fix blocklist entries are allowed to be assigned per policy on basic license #128472

Merged
merged 5 commits into from
Mar 24, 2022
Merged

[Security solution][Endpoint] Fix blocklist entries are allowed to be assigned per policy on basic license #128472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6a737a2
Hide assignment section on blocklsit form when no licensing and isGlo…
dasansol92 Mar 23, 2022
17dbcbe
Fix commented code
dasansol92 Mar 23, 2022
1bae8c9
Merge branch 'main' into feat/olm-the_blocklist_entries_are_allowed_t…
dasansol92 Mar 24, 2022
a3532e6
Don't reset filter when closing flyout
dasansol92 Mar 24, 2022
cae242a
Fix policy selectio was cleaned when switching from by policy to glob…
dasansol92 Mar 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions ...y_solution/public/management/components/artifact_list_page/components/artifact_flyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,10 @@ export const ArtifactFlyout = memo<ArtifactFlyoutProps>(
}

// `undefined` will cause params to be dropped from url
setUrlParams({ itemId: undefined, show: undefined }, true);
setUrlParams({ ...urlParams, itemId: undefined, show: undefined }, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keep old url params and replace the needed ones. This will keep the search and pagination params after closing the flyout


onClose();
}, [isSubmittingData, onClose, setUrlParams]);
}, [isSubmittingData, onClose, setUrlParams, urlParams]);

const handleFormComponentOnChange: ArtifactFormComponentProps['onChange'] = useCallback(
({ item: updatedItem, isValid }) => {
Expand All @@ -285,12 +285,12 @@ export const ArtifactFlyout = memo<ArtifactFlyoutProps>(
if (isMounted) {
// Close the flyout
// `undefined` will cause params to be dropped from url
setUrlParams({ itemId: undefined, show: undefined }, true);
setUrlParams({ ...urlParams, itemId: undefined, show: undefined }, true);

onSuccess();
}
},
[isEditFlow, isMounted, labels, onSuccess, setUrlParams, toasts]
[isEditFlow, isMounted, labels, onSuccess, setUrlParams, toasts, urlParams]
);

const handleSubmitClick = useCallback(() => {
Expand Down
75 changes: 54 additions & 21 deletions ...ns/security_solution/public/management/pages/blocklist/view/components/blocklist_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import { useLicense } from '../../../../../common/hooks/use_license';
import { isValidHash } from '../../../../../../common/endpoint/service/trusted_apps/validations';
import { isArtifactGlobal } from '../../../../../../common/endpoint/service/artifacts';
import type { PolicyData } from '../../../../../../common/endpoint/types';
import { isGlobalPolicyEffected } from '../../../../components/effected_policy_select/utils';

interface BlocklistEntry {
field: BlocklistConditionEntryField;
Expand Down Expand Up @@ -106,14 +107,34 @@ export const BlockListForm = memo(
const warningsRef = useRef<ItemValidation>({});
const errorsRef = useRef<ItemValidation>({});
const [selectedPolicies, setSelectedPolicies] = useState<PolicyData[]>([]);
const isPlatinumPlus = useLicense().isPlatinumPlus();
const isGlobal = useMemo(() => isArtifactGlobal(item as ExceptionListItemSchema), [item]);
const [wasByPolicy, setWasByPolicy] = useState(!isGlobalPolicyEffected(item.tags));
const [hasFormChanged, setHasFormChanged] = useState(false);

const showAssignmentSection = useMemo(() => {
return (
isPlatinumPlus ||
(mode === 'edit' && (!isGlobal || (wasByPolicy && isGlobal && hasFormChanged)))
);
}, [mode, isGlobal, hasFormChanged, isPlatinumPlus, wasByPolicy]);

// set initial state of `wasByPolicy` that checks if the initial state of the exception was by policy or not
useEffect(() => {
if (!hasFormChanged && item.tags) {
setWasByPolicy(!isGlobalPolicyEffected(item.tags));
}
}, [item.tags, hasFormChanged]);

// select policies if editing
useEffect(() => {
if (hasFormChanged) return;
const policyIds = item.tags?.map((tag) => tag.split(':')[1]) ?? [];
if (!policyIds.length) return;
const policiesData = policies.filter((policy) => policyIds.includes(policy.id));

setSelectedPolicies(policiesData);
}, [item.tags, policies]);
}, [hasFormChanged, item.tags, policies]);

const blocklistEntry = useMemo((): BlocklistEntry => {
if (!item.entries.length) {
Expand Down Expand Up @@ -248,6 +269,7 @@ export const BlockListForm = memo(
isValid: isValid(errorsRef.current),
item: nextItem,
});
setHasFormChanged(true);
},
[validateValues, onChange, item]
);
Expand All @@ -261,6 +283,7 @@ export const BlockListForm = memo(
description: event.target.value,
},
});
setHasFormChanged(true);
},
[onChange, item]
);
Expand All @@ -286,6 +309,7 @@ export const BlockListForm = memo(
isValid: isValid(errorsRef.current),
item: nextItem,
});
setHasFormChanged(true);
},
[validateValues, blocklistEntry, onChange, item]
);
Expand All @@ -302,6 +326,7 @@ export const BlockListForm = memo(
isValid: isValid(errorsRef.current),
item: nextItem,
});
setHasFormChanged(true);
},
[validateValues, onChange, item, blocklistEntry]
);
Expand All @@ -320,6 +345,7 @@ export const BlockListForm = memo(
isValid: isValid(errorsRef.current),
item: nextItem,
});
setHasFormChanged(true);
},
[validateValues, onChange, item, blocklistEntry]
);
Expand All @@ -341,6 +367,7 @@ export const BlockListForm = memo(
isValid: isValid(errorsRef.current),
item: nextItem,
});
setHasFormChanged(true);
},
[validateValues, onChange, item, blocklistEntry]
);
Expand All @@ -351,16 +378,20 @@ export const BlockListForm = memo(
? [GLOBAL_ARTIFACT_TAG]
: change.selected.map((policy) => `${BY_POLICY_ARTIFACT_TAG_PREFIX}${policy.id}`);

setSelectedPolicies(change.selected);
const nextItem = { ...item, tags };

// Preserve old selected policies when switching to global
if (!change.isGlobal) {
setSelectedPolicies(change.selected);
}
Comment on lines +383 to +386
Copy link
Member

Choose a reason for hiding this comment

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

Is it east enough to add a test for this bug? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no test file currently for blocklist form, I think @joeypoon was working on it, is it right?

Copy link
Member

@ashokaditya ashokaditya Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Should add the test IMHO. Later, if not in this PR.

validateValues(nextItem);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes an error where the form was enabled for submit when changing the assignment section even there was no name or values already set.

onChange({
isValid: isValid(errorsRef.current),
item: {
...item,
tags,
},
item: nextItem,
});
setHasFormChanged(true);
},
[onChange, item]
[validateValues, onChange, item]
);

return (
Expand Down Expand Up @@ -461,20 +492,22 @@ export const BlockListForm = memo(
/>
</EuiFormRow>

<>
<EuiHorizontalRule />
<EuiFormRow fullWidth>
<EffectedPolicySelect
isGlobal={isArtifactGlobal(item as ExceptionListItemSchema)}
isPlatinumPlus={useLicense().isPlatinumPlus()}
selected={selectedPolicies}
options={policies}
onChange={handleOnPolicyChange}
isLoading={policiesIsLoading}
description={POLICY_SELECT_DESCRIPTION}
/>
</EuiFormRow>
</>
{showAssignmentSection && (
<>
<EuiHorizontalRule />
<EuiFormRow fullWidth>
<EffectedPolicySelect
isGlobal={isGlobal}
isPlatinumPlus={isPlatinumPlus}
selected={selectedPolicies}
options={policies}
onChange={handleOnPolicyChange}
isLoading={policiesIsLoading}
description={POLICY_SELECT_DESCRIPTION}
/>
</EuiFormRow>
</>
)}
</EuiForm>
);
}
Expand Down