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.

Sign up for GitHub

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
Merged

[Security Solution][Endpoint][Admin] Malware Protections Notify User Version #81415

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d17f05a
malware protections notify user versioning
parkiino Oct 21, 2020
8fb2a9f
Merge remote-tracking branch 'upstream/master' into task/malware-noti…
parkiino Oct 26, 2020
9644f13
new component for popup version
parkiino Oct 26, 2020
ce30cdc
change endpoint to agent text
parkiino Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions ...ecurity_solution/public/management/pages/policy/view/policy_forms/protections/malware.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
EuiRadio,
EuiSwitch,
EuiTitle,
EuiText,
EuiSpacer,
htmlIdGenerator,
EuiCallOut,
Expand All @@ -28,6 +29,7 @@ import { policyConfig } from '../../../store/policy_details/selectors';
import { usePolicyDetailsSelector } from '../../policy_hooks';
import { clone } from '../../../models/policy_details_config';
import { LinkToApp } from '../../../../../../common/components/endpoint/link_to_app';
import { popupVersionsMap } from './popup_options_to_versions';

const ProtectionRadioGroup = styled.div`
display: flex;
Expand Down Expand Up @@ -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

<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

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

</EuiText>
<EuiSpacer size="s" />
<EuiCheckbox
id="xpack.securitySolution.endpoint.policyDetail.malware.userNotification"
onChange={handleUserNotificationCheckbox}
checked={userNotificationSelected}
label={i18n.translate(
'xpack.securitySolution.endpoint.policyDetail.malware.userNotification',
{ defaultMessage: 'User Notification' }
)}
label={i18n.translate('xpack.securitySolution.endpoint.policyDetail.malware.notifyUser', {
defaultMessage: 'Notify User',
})}
/>
</>
);
Expand Down
18 changes: 18 additions & 0 deletions ...public/management/pages/policy/view/policy_forms/protections/popup_options_to_versions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* 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 👍


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.

defaultMessage: '7.11+',
}),
],
];

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);