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.

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] User can add new event filter from event filter list #98118

Conversation

dasansol92
Copy link
Contributor

@dasansol92 dasansol92 commented Apr 23, 2021

Summary

This includes a new flyout displayed on administration/event_filters tab when trying to add/update an event filter item.
Once an entry is created, it is added to the entries list (redux store).
This flyout allow user edit the OS type on event filter form.
It also includes unit tests.

Needs eventFilteringEnabled feature flag enabled.

event filter flyout PR

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dasansol92 dasansol92 added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v7.14.0 labels Apr 23, 2021
@dasansol92 dasansol92 force-pushed the feature/olm-user_can_add_new_event_filter_from_event_filter_list-955 branch from 70a7c3d to dd7e246 Compare April 26, 2021 09:33
@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

@dasansol92 dasansol92 marked this pull request as ready for review April 26, 2021 12:08
@dasansol92 dasansol92 requested a review from a team as a code owner April 26, 2021 12:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

@dasansol92 dasansol92 force-pushed the feature/olm-user_can_add_new_event_filter_from_event_filter_list-955 branch from 6571bee to 74f9c91 Compare April 26, 2021 15:09
@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

@@ -51,57 +51,66 @@ export interface AuthenticatedElasticUser {
}

export const useCurrentUser = (): AuthenticatedElasticUser | null => {
const isMounted = useRef(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for fixing this!

@kevinlog
Copy link
Contributor

Flyout and functionality looks good!

Without Endpoint added, we still have a working form which mirrors Exceptions

image

After index creation, we get the fields

image

successful creation:
image

Copy link
Contributor

@kevinlog kevinlog left a comment

Choose a reason for hiding this comment

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

🔥 🚢

@dasansol92 dasansol92 force-pushed the feature/olm-user_can_add_new_event_filter_from_event_filter_list-955 branch from 85b78d8 to 5b3e7e5 Compare April 27, 2021 16:26
@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Looks good. Left some comments, but they are not critical enought to hold merging this in.

import { EuiButton, EuiEmptyPrompt } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';

const EmptyPrompt = styled(EuiEmptyPrompt)`
Copy link
Contributor

Choose a reason for hiding this comment

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

What was this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the default max-width is too short for the text inside

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I would not set the max-width but rather let the text wrap instead. Attempting to make the element wide enough to accommodate the english version of the text show is not a good approach because the text can be localized and thus the length of the message might change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally right!

…lter_list-955' of github.com:dasansol92/kibana into feature/olm-user_can_add_new_event_filter_from_event_filter_list-955
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2172 2174 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 7.0MB 7.0MB +11.7KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢

Thanks for the changes. Looks good to merge in my opinion. I need this PR in order to make some progress with mine, since this one already puts in place some of the pieces I need

@@ -118,6 +119,26 @@ const normalizeTrustedAppsPageLocation = (
}
};

const normalizeEventFiltersPageLocation = (
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 I wonder if we can create a generic utility that could work for any use case in parsing/normalizing url params, so that we did not have to keep cloning this code. maybe a utility that takes in the defaults ++ the actual (aka: location) value and returns back the location type of information. Likely would be a TS generic function so that it can work across use cases.

@dasansol92 dasansol92 merged commit 1ac3c57 into elastic:master Apr 28, 2021
dasansol92 added a commit to dasansol92/kibana that referenced this pull request Apr 28, 2021
…ist (elastic#98118)

* New flyout with event filters form

* Changes on event filters form to allow OS selector. Add new error on state for OS. Add created entry to the entries list

* Fixes typo

* Adds empty page with an add button that opens flyout. Alos added route and path management

* Fixes type and adds a TODO comment. Also removes ESlit rule for useCallback deps

* Fixes unit test. Adds consts for default page size and page index

* Fixes warning state update on an unmounted component

* Fixes infinite useEffect loop useFetchIndex hook because non memoized value

* Adds policy:all to eventFilter.tag and disables or button on exception builder

* Changes component name and simplify hook using useCallback without a custom callback

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
dasansol92 added a commit that referenced this pull request Apr 28, 2021
…ist (#98118) (#98604)

* New flyout with event filters form

* Changes on event filters form to allow OS selector. Add new error on state for OS. Add created entry to the entries list

* Fixes typo

* Adds empty page with an add button that opens flyout. Alos added route and path management

* Fixes type and adds a TODO comment. Also removes ESlit rule for useCallback deps

* Fixes unit test. Adds consts for default page size and page index

* Fixes warning state update on an unmounted component

* Fixes infinite useEffect loop useFetchIndex hook because non memoized value

* Adds policy:all to eventFilter.tag and disables or button on exception builder

* Changes component name and simplify hook using useCallback without a custom callback

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
madirey pushed a commit to madirey/kibana that referenced this pull request May 11, 2021
…ist (elastic#98118)

* New flyout with event filters form

* Changes on event filters form to allow OS selector. Add new error on state for OS. Add created entry to the entries list

* Fixes typo

* Adds empty page with an add button that opens flyout. Alos added route and path management

* Fixes type and adds a TODO comment. Also removes ESlit rule for useCallback deps

* Fixes unit test. Adds consts for default page size and page index

* Fixes warning state update on an unmounted component

* Fixes infinite useEffect loop useFetchIndex hook because non memoized value

* Adds policy:all to eventFilter.tag and disables or button on exception builder

* Changes component name and simplify hook using useCallback without a custom callback

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants