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][Endpoint] Fix blocklist entries are allowed to be assigned per policy on basic license #128472

Conversation

dasansol92
Copy link
Contributor

Summary

  • Fix blocklist entry is not allowed to be assigned per policy on basic license:

blocklist by policy no license after

  • Fix url filter param was removed when closing flyout:

Before:
blocklist reset url params when close flyout before

After:
blocklist reset url params when close flyout after

For maintainers

@dasansol92 dasansol92 added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.2.0 labels Mar 24, 2022
@dasansol92 dasansol92 requested a review from a team as a code owner March 24, 2022 11:14
@elasticmachine
Copy link
Contributor

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

setSelectedPolicies(change.selected);
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.

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

Copy link
Member

@ashokaditya ashokaditya 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 finding and fixing these bugs.
I found another one that I noticed while running your branch.

Switching policy assignment views makes the checks disappear.
Here's a clip.

blocklist

PS.
I also notice that if I save the data on the flyout after the checkbox disappears on the policy assignment switch, it actually does save it as a global aritfact.

@dasansol92
Copy link
Contributor Author

@joeypoon Did you know about this policy bug Ash has mention above?

@joeypoon
Copy link
Member

@joeypoon Did you know about this policy bug Ash has mention above?

I was not aware, no. I can take a look at it (probably Monday).

@dasansol92
Copy link
Contributor Author

@joeypoon I'll include the fix on this pr 🙂

@dasansol92
Copy link
Contributor Author

@ashokaditya @joeypoon Fixed here:

keep old policy selction for blocklist form

@dasansol92 dasansol92 requested a review from ashokaditya March 24, 2022 14:51
Comment on lines +383 to +386
// Preserve old selected policies when switching to global
if (!change.isGlobal) {
setSelectedPolicies(change.selected);
}
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

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.

Copy link
Member

@joeypoon joeypoon 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. thanks for fixing these bugs 🙏

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 4.8MB 4.8MB +355.0B

History

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

@dasansol92 dasansol92 merged commit 8ada3b3 into elastic:main Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants