[SECURITY_SOLUTION] delete advanced Policy fields when they are empty #84368

kevinlog · 2020-11-25T20:06:21Z

Summary

In the advanced Policy UI, we need to ensure that when fields are cleared out, we delete the relevant section in the advanced fields. Further, we need to ensure that we clean up any left over nested fields.

Initially, you will see no advanced section in the Policy yaml in Fleet:

When a user adds "false" to a field, we should see this in the Policy yaml in Fleet (note that the field added is nested):

After the user deletes the field, the path is empty, everything should be cleaned up before going to the Agent/Endpoint:

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

kevinlog · 2020-11-30T14:08:25Z

@elasticmachine merge upstream

nnamdifrankie · 2020-11-30T16:54:54Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+        break; // We are looking at a non-empty section, so we can terminate.
+      }
+    }
+  }


Can this be covered by testing

yes, I can write a test case for it

pushed up a test

paul-tavares

Looks good. Had only some questions for my own knowning :)

paul-tavares · 2020-11-30T17:03:59Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+      if (
+        newPolicyConfig[nextPath[nextPath.length - 1]] === undefined ||
+        newPolicyConfig[nextPath[nextPath.length - 1]] === '' ||
+        Object.keys(newPolicyConfig[nextPath[nextPath.length - 1]] as object).length === 0


I'm assuming this casting here is safe at runtime? will it always be an object if defined at this point?

'as object' ? Shouldn't it be 'as Object' ?

I think both work. The type error I had on this went away after I added the cast

paul-tavares · 2020-11-30T17:08:10Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+        Object.keys(newPolicyConfig[nextPath[nextPath.length - 1]] as object).length === 0
+      ) {
+        if (nextPath[nextPath.length - 1] === 'advanced') {
+          newPolicyConfig[nextPath[nextPath.length - 1]] = undefined;


If I'm reading this right, it seems that if the key name is advanced we keep it, but set it to undefined, but any other key we delete it. Should the advanced key also be deleted? Or does that happen when you walk the path back to the prior level?

the UI looks for an advanced key which will be initialized to undefined to start making decisions on how to render the form. This is essentially resetting if it's completely emptied out

i'm also a little confused about the if/else logic here; maybe more comments would be clarifying

added more comments

efreeti · 2020-11-30T17:40:28Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+        } else {
+          delete newPolicyConfig[nextPath[nextPath.length - 1]];
+        }
+        newPolicyConfig = obj;


I see this assignment multiple times, what does it do?

it updates the reference to the area in the config as we follow the given path. Say our path is linux.advanced.config - we start at linux and access advanced, then we need to update the reference to access config in the next iteration.

jmiller263 · 2020-11-30T18:57:53Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

  for (let i = 0; i < path.length - 1; i++) {
    if (!newPolicyConfig[path[i]]) {
      newPolicyConfig[path[i]] = {} as Record<string, unknown>;
    }
    newPolicyConfig = newPolicyConfig[path[i]] as Record<string, unknown>;
  }
  newPolicyConfig[path[path.length - 1]] = value;
+
+  // Then, if the user is deleting the value, then we need to ensure we clean up the config.
+  // We delete any sections are the empty, whether that be an empty string,  empty object, or undefined.


s/are the/that are

jmiller263 · 2020-11-30T19:27:10Z

x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx

+        Object.keys(newPolicyConfig[nextPath[nextPath.length - 1]] as object).length === 0
+      ) {
+        if (nextPath[nextPath.length - 1] === 'advanced') {
+          newPolicyConfig[nextPath[nextPath.length - 1]] = undefined;


i'm also a little confused about the if/else logic here; maybe more comments would be clarifying

…kevinlog/kibana into bug/delete-empty-advanced-policy-fields

kevinlog · 2020-12-02T18:13:57Z

@elasticmachine merge upstream

kibanamachine · 2020-12-02T18:14:08Z

expected head sha didn’t match current head ref.

kevinlog · 2020-12-03T13:32:39Z

@elasticmachine merge upstream

kibanamachine · 2020-12-03T15:12:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b943055

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`	8.0MB	8.0MB	+572.0B

History

💚 Build #91470 succeeded c601ea8
💚 Build #91334 succeeded 1b3d631
💔 Build #91236 failed 58ee34b
💔 Build #91164 failed fd115dd
💔 Build #91101 failed c746fc8

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

…#84368) (#84917)

* master: (28 commits) [Actions] fixes bug where severity is auto selected but not applied to the action in PagerDuty (elastic#84891) Only attempt to rollover signals index if version < builtin version (elastic#84982) skip flaky suite (elastic#84978) skip lens rollup tests Add geo containment tracking alert type (elastic#84151) Changed rollup tests to use test user rather than elastic super user. (elastic#79567) skip 'should allow creation of lens xy chart' elastic#84957 [APM] Add log_level/sanitize_field_names config options to Python Agent (elastic#84810) [Maps] geo line source (elastic#76572) [data.search] Move search method inside session service and add tests (elastic#84715) skip lens drag and drop test. elastic#84941 [Ingest Node Pipelines] Integrate painless autocomplete (elastic#84554) [Lens] allow drag and drop reorder on xyChart for y dimension (elastic#84640) [Lens] Fix error when selecting the current field again (elastic#84817) [Metrics UI] Add metadata tab to node details flyout (elastic#84454) [CI] Enables APM collection (elastic#81731) [Workplace Search] Migrate Sources Schema tree (elastic#84847) Disable checking for conflicts when copying saved objects (elastic#83575) [SECURITY_SOLUTION] delete advanced Policy fields when they are empty (elastic#84368) y18n 4.0.0 -> 4.0.1 (elastic#84905) ...

kevinlog added 4 commits November 25, 2020 14:59

delete advanced fields when they are empty

4307a9f

fix types

d56f0e3

clean up logic

e982777

clean up

9ac26da

kevinlog marked this pull request as ready for review November 30, 2020 14:07

kevinlog requested review from a team as code owners November 30, 2020 14:07

kevinlog added v7.11.0 release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution labels Nov 30, 2020

Merge branch 'master' into bug/delete-empty-advanced-policy-fields

ee92d62

kevinlog mentioned this pull request Nov 30, 2020

Advanced Policy UI in Endpoint Security should delete empty entries in Package Policy #84127

Closed

nnamdifrankie reviewed Nov 30, 2020

View reviewed changes

paul-tavares approved these changes Nov 30, 2020

View reviewed changes

efreeti reviewed Nov 30, 2020

View reviewed changes

jmiller263 approved these changes Nov 30, 2020

View reviewed changes

parkiino approved these changes Nov 30, 2020

View reviewed changes

kevinlog added 6 commits December 1, 2020 07:57

more comments and test

b620c6e

Merge branch 'bug/delete-empty-advanced-policy-fields' of github.com:…

a488234

…kevinlog/kibana into bug/delete-empty-advanced-policy-fields

fix test

c746fc8

adjust test

fd115dd

fix test

58ee34b

fix typo

1b3d631

Merge branch 'master' into bug/delete-empty-advanced-policy-fields

c601ea8

Merge branch 'master' into bug/delete-empty-advanced-policy-fields

b943055

kevinlog merged commit ac71d2e into elastic:master Dec 3, 2020

kevinlog deleted the bug/delete-empty-advanced-policy-fields branch December 3, 2020 15:57

kevinlog mentioned this pull request Dec 3, 2020

[7.x] [SECURITY_SOLUTION] delete advanced Policy fields when they are empty (#84368) #84917

Merged

kevinlog added a commit that referenced this pull request Dec 3, 2020

[SECURITY_SOLUTION] delete advanced Policy fields when they are empty (…

456ec18

…#84368) (#84917)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SECURITY_SOLUTION] delete advanced Policy fields when they are empty #84368

[SECURITY_SOLUTION] delete advanced Policy fields when they are empty #84368

kevinlog commented Nov 25, 2020 •

edited

Loading

kevinlog commented Nov 30, 2020

nnamdifrankie Nov 30, 2020

kevinlog Nov 30, 2020

kevinlog Dec 1, 2020

paul-tavares left a comment

paul-tavares Nov 30, 2020

kevinlog Nov 30, 2020

efreeti Nov 30, 2020

kevinlog Dec 1, 2020

paul-tavares Nov 30, 2020

kevinlog Nov 30, 2020

jmiller263 Nov 30, 2020

kevinlog Dec 1, 2020

efreeti Nov 30, 2020

kevinlog Dec 1, 2020

jmiller263 Nov 30, 2020

kevinlog Dec 1, 2020

jmiller263 Nov 30, 2020

kevinlog commented Dec 2, 2020

kibanamachine commented Dec 2, 2020

kevinlog commented Dec 3, 2020

kibanamachine commented Dec 3, 2020

[SECURITY_SOLUTION] delete advanced Policy fields when they are empty #84368

[SECURITY_SOLUTION] delete advanced Policy fields when they are empty #84368

Conversation

kevinlog commented Nov 25, 2020 • edited Loading

Summary

Checklist

kevinlog commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinlog commented Dec 2, 2020

kibanamachine commented Dec 2, 2020

kevinlog commented Dec 3, 2020

kibanamachine commented Dec 3, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kevinlog commented Nov 25, 2020 •

edited

Loading