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][Admin] Malware Protections Notify User Version #81415

Merged
merged 4 commits into from
Oct 27, 2020

Conversation

parkiino
Copy link
Contributor

@parkiino parkiino commented Oct 21, 2020

Summary

  • Adds endpoint version number that malware protections user notification checkbox is valid for in policy details

ISSUE: https://github.com/elastic/security-team/issues/337

image

@parkiino parkiino added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Management Feature:Endpoint Elastic Endpoint feature v7.11.0 labels Oct 21, 2020
@parkiino parkiino requested review from a team as code owners October 21, 2020 23:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-management (Team:Endpoint Management)

@@ -189,14 +191,17 @@ export const MalwareProtections = React.memo(() => {
/>
</h6>
</EuiTitle>
<EuiText color="subdued" size="xs">
<i>{`Endpoint versions ${popupVersionsMap.get('malware')}`}</i>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this copy should refer to Agent versions over Endpoint. Users in Fleet will upgrade Agents as opposed to just Endpoints.

What are your thoughts @bfishel @caitlinbetz

* you may not use this file except in compliance with the Elastic License.
*/

import { i18n } from '@kbn/i18n';
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 should just have this file be the place we store all of the version information for protections.

We'd eventually add Ransomware and Register as AV. So we could rename this to just be policy_options_to_versions.ts

@paul-tavares @efreeti let me know your thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed @kevinlog 👍

* you may not use this file except in compliance with the Elastic License.
*/

import { i18n } from '@kbn/i18n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed @kevinlog 👍

const popupVersions: Array<[string, string]> = [
[
'malware',
i18n.translate('xpack.securitySolution.endpoint.policyDetails.popup.version.7.11', {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the need to i18n this if all we are going to store the version.

],
];

export const popupVersionsMap = new Map<string, string>(popupVersions);
Copy link
Contributor

Choose a reason for hiding this comment

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

add typing so that Map is set to readonly`

Suggested change
export const popupVersionsMap = new Map<string, string>(popupVersions);
export const popupVersionsMap: ReadonlyMap<string, string> = new Map(popupVersions);

@@ -189,14 +191,17 @@ export const MalwareProtections = React.memo(() => {
/>
</h6>
</EuiTitle>
<EuiText color="subdued" size="xs">
Copy link
Contributor

Choose a reason for hiding this comment

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

The question longer term would be: should we add this block of JSX manually to every new policy field or have it abstracted behind a component that will determine whether it should be shown or not just by the near existance of that prop name in the "field-to-version" map file.

Here is what I mean:
We could use a component like this:

const SupportedVersionNotice = ({optionName}) => {
  const version = popupVersionsMap.get(optionName);
  if (!version) {
    return null;
  }
  return <EuiText color="subdued" size="xs">
      <FormattedMessage id="...." defaultMessage="Endpoint versions {version}" values={{ version }} />
    </EuiText>
}

using it:

<SupportedVersionNotice optionName="malware" />

And if we ever removed this options from the translation map file, then the text in the UI would just "magically" go way :)

Just an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that sounds like a noice idea. i can do that

@@ -189,14 +191,17 @@ export const MalwareProtections = React.memo(() => {
/>
</h6>
</EuiTitle>
<EuiText color="subdued" size="xs">
<i>{`Endpoint versions ${popupVersionsMap.get('malware')}`}</i>
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be i18n'd (I think maybe you had this in the map file at one point, then moved it here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah okie that makes sense

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.

🔥 🚀 🚢

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
securitySolution 2062 2063 +1

async chunks size

id before after diff
securitySolution 8.1MB 8.1MB +915.0B

History

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

@parkiino parkiino merged commit 8c07eb9 into elastic:master Oct 27, 2020
@parkiino parkiino deleted the task/malware-notification-version branch October 27, 2020 13:58
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
* master:
  [Security Solution][Endpoint][Admin] Malware Protections Notify User Version (elastic#81415)
  Revert "[Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)"
  [Maps] Use default format when proxying EMS-files (elastic#79760)
  [Discover] Deangularize context.html (elastic#81442)
  Use the displayName property in default editor (elastic#73311)
  adds bug label to Bug report for Security Solution template (elastic#81643)
  [ML] Transforms: Remove index field limitation for custom query. (elastic#81467)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)
  [Task Manager] Mark task as failed if maxAttempts has been met. (elastic#80681)
  [Lens] Fix URL query loss on redirect (elastic#81475)
  Log reason for 404 in field existence check (elastic#81315)
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Oct 27, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
* master: (87 commits)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81778)
  [i18n] add get_kibana_translation_paths tests (elastic#81624)
  [UX] Fix search term reset from url (elastic#81654)
  [Lens] Improved range formatter (elastic#80132)
  [Resolver] `SideEffectContext` changes, remove `@testing-library` uses (elastic#81077)
  [Time to Visualize] Update Library Text with Call to Action (elastic#81667)
  [docs] Resolving failed Kibana upgrade migrations (elastic#80999)
  [ftr/menuToggle] provide helper for enhanced menu toggle handling (elastic#81709)
  [Task Manager] adds basic observability into Task Manager's runtime operations (elastic#77868)
  Doc changes for stack management and grouped feature privileges (elastic#80486)
  Added functional test for alerts list filters by status, alert type and action type. Did a code refactoring and splitting for alerts tests. (elastic#81422)
  [Security Solution][Endpoint][Admin] Malware Protections Notify User Version (elastic#81415)
  Revert "[Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)"
  [Maps] Use default format when proxying EMS-files (elastic#79760)
  [Discover] Deangularize context.html (elastic#81442)
  Use the displayName property in default editor (elastic#73311)
  adds bug label to Bug report for Security Solution template (elastic#81643)
  [ML] Transforms: Remove index field limitation for custom query. (elastic#81467)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)
  [Task Manager] Mark task as failed if maxAttempts has been met. (elastic#80681)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 28, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

parkiino added a commit that referenced this pull request Oct 28, 2020
…Version (#81415) (#81825)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants