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] Remove use of Redux from the Endpoint Policy Settings form #161511

Merged

Conversation

paul-tavares
Copy link
Contributor

@paul-tavares paul-tavares commented Jul 8, 2023

Summary

  • Re-creates all policy settings form components so that the Policy settings are provided as a prop
  • Adds mode = view to the policy form. When in this mode (user has no authz to edit), form will be displayed in view only mode (no more disabled form elements)

NOTE ⚠️ ⚠️
PR does not include unit tests for new form components. A follow up PR will add those. Existing cypress and FTR tests, however, have been adjusted to run with the revised code

Screen captures

olm-remove-redux-from-policy-details

Locked cards when license is lower than Platinum

image

User has Read only access to Endpoint Policies

(test role: hunter)

image

DEV TODO

Checklist

  • Manual tests:
    • Validate view mode in security solution
    • ensure that user with no Endpoint security edit policy can still edit it in Fleet if they have fleet Edit permissions
    • validate lower license and locked form cards
    • Validate edit form in fleet
  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Unit or functional tests were updated or added to match the most common scenarios

Sorry, something went wrong.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@paul-tavares paul-tavares marked this pull request as ready for review July 11, 2023 19:26
@paul-tavares paul-tavares requested a review from a team as a code owner July 11, 2023 19:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

@paul-tavares paul-tavares requested review from ashokaditya and removed request for dasansol92 July 11, 2023 19:27
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 the changes. I left some questions and nits. None of them should block 🚢 ing this.

Comment on lines +68 to +106
if (os === 'windows') {
newPayload[os][protection].mode = ProtectionModes.off;
} else if (os === 'mac') {
newPayload[os][protection as MacPolicyProtection].mode = ProtectionModes.off;
} else if (os === 'linux') {
newPayload[os][protection as LinuxPolicyProtection].mode = ProtectionModes.off;
}
if (isPlatinumPlus) {
if (os === 'windows') {
newPayload[os].popup[protection].enabled = event.target.checked;
} else if (os === 'mac') {
newPayload[os].popup[protection as MacPolicyProtection].enabled =
event.target.checked;
} else if (os === 'linux') {
newPayload[os].popup[protection as LinuxPolicyProtection].enabled =
event.target.checked;
}
}
}
} else {
for (const os of osList) {
if (os === 'windows') {
newPayload[os][protection].mode = ProtectionModes.prevent;
} else if (os === 'mac') {
newPayload[os][protection as MacPolicyProtection].mode = ProtectionModes.prevent;
} else if (os === 'linux') {
newPayload[os][protection as LinuxPolicyProtection].mode = ProtectionModes.prevent;
}
if (isPlatinumPlus) {
if (os === 'windows') {
newPayload[os].popup[protection].enabled = event.target.checked;
} else if (os === 'mac') {
newPayload[os].popup[protection as MacPolicyProtection].enabled =
event.target.checked;
} else if (os === 'linux') {
newPayload[os].popup[protection as LinuxPolicyProtection].enabled =
event.target.checked;
}
}
Copy link
Member

@ashokaditya ashokaditya Jul 12, 2023

Choose a reason for hiding this comment

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

You can extract this repeated logic as functions outside of the component. Something like

const setProtectionsPopup = (
  os: 'windows' | 'mac' | 'linux',
  protection: PolicyProtection,
  payload: PolicyConfig,
  value: boolean
) => {
  if (os === 'windows') {
    payload[os].popup[protection].enabled = value;
  } else if (os === 'mac') {
    payload[os].popup[protection as MacPolicyProtection].enabled = value;
  } else if (os === 'linux') {
    payload[os].popup[protection as LinuxPolicyProtection].enabled = value;
  }
};

const setProtectionModes = (
  os: 'windows' | 'mac' | 'linux',
  protection: PolicyProtection,
  payload: PolicyConfig,
  value: ProtectionModes
) => {
  if (os === 'windows') {
    payload[os][protection].mode = value;
  } else if (os === 'mac') {
    payload[os][protection as MacPolicyProtection].mode = value;
  } else if (os === 'linux') {
    payload[os][protection as LinuxPolicyProtection].mode = value;
  }
};

and then use it here

 if (event.target.checked === false) {
  for (const os of osList) {
    setProtectionModes(os, protection, newPayload, ProtectionModes.off);
    if (isPlatinumPlus) {
      setProtectionsPopup(os, protection, newPayload, false);
    }
  }
} else {
  for (const os of osList) {
    setProtectionModes(os, protection, newPayload, ProtectionModes.prevent);
    if (isPlatinumPlus) {
      setProtectionsPopup(os, protection, newPayload, 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.

same comment here. I wanted to leave this logic as is until I have tests in place. Then I would like to circle back and simplify the code and make it more efficient.

Comment on lines 149 to 153
if (protectionMode === ProtectionModes.prevent) {
newPayload[os].popup[protection].enabled = true;
} else {
newPayload[os].popup[protection].enabled = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: This can also be written as

newPayload[os].popup[protection].enabled = protectionMode === ProtectionModes.prevent;

Same goes for the other ones below if you change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep this logic unchanged, so I just copied it as is. I do want to come back around (after tests are introduced - in next PR) to possibly simplify allot of this logic and maybe use lodash's set() to update values.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4274 4273 -1

Async chunks

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

id before after diff
securitySolution 11.2MB 9.8MB -1.4MB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 53.7KB 53.6KB -30.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 14 16 +2
securitySolution 409 415 +6
total +8

Total ESLint disabled count

id before after diff
enterpriseSearch 15 17 +2
securitySolution 488 494 +6
total +8

History

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

cc @paul-tavares

@paul-tavares paul-tavares merged commit 3ba51e4 into elastic:main Jul 12, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 12, 2023
@paul-tavares paul-tavares deleted the task/olm-remove-redux-from-policy-details branch July 12, 2023 19:23
szwarckonrad added a commit that referenced this pull request Aug 23, 2023
… a refresh (#164403)

Closes elastic/security-team#7386

Dispatch an action that will update the stored policy upon successful
saving of Policy Settings. This should occur only when no 'routeState'
is being set, as in such a scenario, a page change triggers a refetch on
its own.

Even though redux was removed from Policy Settings in
#161511 , the policy object that
feeds the view is being fetched as a result of onUrlChange action and
stored in the redux store. This should be addressed in the future.


https://github.com/elastic/kibana/assets/29123534/cf008d0e-804a-49f9-a2f7-9bb7d1162b28
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 23, 2023
… a refresh (elastic#164403)

Closes elastic/security-team#7386

Dispatch an action that will update the stored policy upon successful
saving of Policy Settings. This should occur only when no 'routeState'
is being set, as in such a scenario, a page change triggers a refetch on
its own.

Even though redux was removed from Policy Settings in
elastic#161511 , the policy object that
feeds the view is being fetched as a result of onUrlChange action and
stored in the redux store. This should be addressed in the future.

https://github.com/elastic/kibana/assets/29123534/cf008d0e-804a-49f9-a2f7-9bb7d1162b28
(cherry picked from commit 5eca861)
kibanamachine added a commit that referenced this pull request Aug 23, 2023
…t until a refresh (#164403) (#164557)

# Backport

This will backport the following commits from `main` to `8.10`:
- [[Defend Workflows]Changes to policy settings are not persistent until
a refresh (#164403)](#164403)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Konrad
Szwarc","email":"konrad.szwarc@elastic.co"},"sourceCommit":{"committedDate":"2023-08-23T10:46:04Z","message":"[Defend
Workflows]Changes to policy settings are not persistent until a refresh
(#164403)\n\nCloses
https://github.com/elastic/security-team/issues/7386\r\n\r\nDispatch an
action that will update the stored policy upon successful\r\nsaving of
Policy Settings. This should occur only when no 'routeState'\r\nis being
set, as in such a scenario, a page change triggers a refetch on\r\nits
own.\r\n\r\nEven though redux was removed from Policy Settings
in\r\nhttps://github.com//pull/161511 , the policy object
that\r\nfeeds the view is being fetched as a result of onUrlChange
action and\r\nstored in the redux store. This should be addressed in the
future.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/29123534/cf008d0e-804a-49f9-a2f7-9bb7d1162b28","sha":"5eca8618eab9e6f35b1d13d9c6ae8b1df598ad5c","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Defend
Workflows","v8.10.0","v8.11.0"],"number":164403,"url":"https://github.com/elastic/kibana/pull/164403","mergeCommit":{"message":"[Defend
Workflows]Changes to policy settings are not persistent until a refresh
(#164403)\n\nCloses
https://github.com/elastic/security-team/issues/7386\r\n\r\nDispatch an
action that will update the stored policy upon successful\r\nsaving of
Policy Settings. This should occur only when no 'routeState'\r\nis being
set, as in such a scenario, a page change triggers a refetch on\r\nits
own.\r\n\r\nEven though redux was removed from Policy Settings
in\r\nhttps://github.com//pull/161511 , the policy object
that\r\nfeeds the view is being fetched as a result of onUrlChange
action and\r\nstored in the redux store. This should be addressed in the
future.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/29123534/cf008d0e-804a-49f9-a2f7-9bb7d1162b28","sha":"5eca8618eab9e6f35b1d13d9c6ae8b1df598ad5c"}},"sourceBranch":"main","suggestedTargetBranches":["8.10"],"targetPullRequestStates":[{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/164403","number":164403,"mergeCommit":{"message":"[Defend
Workflows]Changes to policy settings are not persistent until a refresh
(#164403)\n\nCloses
https://github.com/elastic/security-team/issues/7386\r\n\r\nDispatch an
action that will update the stored policy upon successful\r\nsaving of
Policy Settings. This should occur only when no 'routeState'\r\nis being
set, as in such a scenario, a page change triggers a refetch on\r\nits
own.\r\n\r\nEven though redux was removed from Policy Settings
in\r\nhttps://github.com//pull/161511 , the policy object
that\r\nfeeds the view is being fetched as a result of onUrlChange
action and\r\nstored in the redux store. This should be addressed in the
future.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/29123534/cf008d0e-804a-49f9-a2f7-9bb7d1162b28","sha":"5eca8618eab9e6f35b1d13d9c6ae8b1df598ad5c"}}]}]
BACKPORT-->

Co-authored-by: Konrad Szwarc <konrad.szwarc@elastic.co>
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.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants