[EDR Workflows][Serverless] Gate custom note by szwarckonrad · Pull Request #193171 · elastic/kibana

szwarckonrad · 2024-09-17T13:41:54Z

This PR implements tier-based gating for custom notification messages in Protections. Only users on the Endpoint Complete tier will have the ability to modify these messages, while users on the Endpoint Essentials tier will no longer have this capability. If a user on the Essentials tier had made any changes to custom notifications before this update, those messages will be reset to the default ones.

The changes are applied in three areas:

UI - An upsell banner is displayed for Essentials users.
API - We now prevent API calls that attempt to set or modify custom notification messages for Essentials users.
Policy Watcher - Upon Kibana startup (e.g., after a downgrade), we validate all policies for tier compliance. If a policy contains a custom notification message and the user is on the Essentials tier, the message will be reset to the default.

…om-note

szwarckonrad · 2024-09-23T08:54:01Z

+    return policy;
  };

+  const generateExpectedPolicyMock = ({


I decided to keep this fragment hardcoded, as any attempts to generate it with iterations resulted in code that, in my opinion, would be difficult to read and hard for anyone to understand.

In that case, I would urge you to consider adding this info as a comment block so this block stays that way.

elasticmachine · 2024-09-23T08:57:41Z

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

semd

Feature PLI config and upselling component registration LGTM

paul-tavares

Looks good 👍

ashokaditya

Looks good. I left some suggestions for you to consider, esp. with the info text.

From your screenshot, I noticed that the bottom padding/spacing is more than the left/right padding. Is there a way to ensure they are consistent? Perhaps use xs instead of m wherever NotifyUserOption is used. I know this was not something you changed but it's nice to update.

ashokaditya · 2024-09-26T08:15:13Z

+          cy.getByTestSubj(`endpointPolicyForm-${protection}-notifyUser-customMessage`)
+            .should('exist')
+            .and('be.visible');


For this test and the one above, consider adding a line of comment above each block of assertion for what is visible and what is not. The last assertion here seems like customMessage is visible which I assume is a custom notificaton but the test title contradicts that. It's confusing I think

ashokaditya · 2024-09-26T08:22:08Z

+  'xpack.securitySolutionServerless.endpointCustomNotification.cardMessage',
+  {
+    defaultMessage:
+      'To customize the user notification, you must add Endpoint Protection Complete to your project.',


I'm not sure if this info text is enough to inform the user what they need to do. Maybe you want to check with @caitlinbetz. I would go with something like To enable customized user notifications, you must add Endpoint Complete to your project. I don't see Endpoint Protection Complete anywhere else in the codebase for such info.

Talked with @caitlinbetz offline, we've decided to keep the naming consistent - https://github.com/elastic/security-team/issues/9762#issuecomment-2377746522

In that case, we should update the other upsell info message that we currently have about modifying protection updates that says To modify protection updates, you must add at least Endpoint Complete to your project.

Also consider this concise info-text instead To customize user notifications, you must add Endpoint Protection Complete to your project.

ashokaditya · 2024-09-26T08:22:45Z

      const productFeatureError: Error & { statusCode?: number; apiPassThrough?: boolean } =
        new Error(
-          'To modify protection updates, you must add at least Endpoint Complete to your project.'
+          'To customize the user notification, you must add Endpoint Protection Complete to your project.'


Same about the info text update here

Talked with @caitlinbetz offline, we've decided to keep the naming consistent - https://github.com/elastic/security-team/issues/9762#issuecomment-2377746522

See my above comment.

ashokaditya · 2024-09-26T08:25:18Z

+    return policy;
  };

+  const generateExpectedPolicyMock = ({


In that case, I would urge you to consider adding this info as a comment block so this block stays that way.

ashokaditya · 2024-09-26T08:34:49Z

+          cy.getByTestSubj(`endpointPolicyForm-${protection}-enableDisableSwitch`).click();
+          [
+            'endpointPolicy-customNotificationLockedCard-title',
+            'endpointPolicy-customNotificationLockedCard',
+            'endpointPolicy-customNotificationLockedCard-badge',
+          ].forEach((testSubj) => {
+            cy.getByTestSubj(testSubj, { timeout: 60000 }).should('exist').and('be.visible');
+          });
+          cy.getByTestSubj(`endpointPolicyForm-${protection}-notifyUser-customMessage`).should(
+            'not.exist'
+          );


Same for this test. Please add some comments to explain what the assertions are

ashokaditya · 2024-09-26T09:20:33Z

+              <h4>{NOTIFICATION_MESSAGE_LABEL}</h4>
+            </EuiText>
+            <EuiSpacer size="xs" />
+            <>{userNotificationMessage || getEmptyValue()}</>


Q: Does emptyValue() (-) ever get rendered? from the types looks like userNotificationMessage is always present.

I believe that userNotificationMessage can be an empty string, which works fine when displaying it in the input box. However, in view-only mode, we want to indicate graphically that there is no notification message by displaying -.

Yeah, but I don't see the - in the view only mode. I still see an empty textarea. 😕 I'll test again.

User with full access:

User with read only access:

Seems like empty string is being replaced with the default message here -

kibana/x-pack/plugins/security_solution/public/management/pages/policy/store/policy_details/middleware/policy_settings_middleware.ts

Line 44 in 847285b

if (policyItem.inputs[0].config.policy.value.windows.popup.malware.message === '') {

I didn’t feel comfortable enough removing the || getEmptyValue(), so I decided to leave it in 😉

Yeah, this is updating it to the Elastic Security {action} {filename} text. Which I think should be updated to Elastic Endpoint {action} {filename} if at all that is the behavior we need. Maybe @paul-tavares @dasansol92 know something about this logic?

I’m planning to gather all the improvement suggestions from this PR and consolidate them into a single issue for further discussion. What do you think?

That sounds good to me.

ashokaditya · 2024-09-26T09:21:16Z

+        return <CustomNotificationUpsellingComponent />;
+      }
+
+      if (!isEditMode) {


Does this ever render? I tested it out and looks like it is valid pre-render so - never shows on the screen.

It renders for users who do not have RBAC access to edit custom messages.

# Conflicts: # x-pack/plugins/security_solution/server/endpoint/migrations/turn_off_policy_protections.test.ts # x-pack/plugins/security_solution/server/endpoint/migrations/turn_off_policy_protections.ts

kibana-ci · 2024-10-01T12:37:10Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 48ce7f6
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-193171-48ce7f691542
Security Deployment

Failed CI Steps

FTR Configs #42

Test Failures

[job] [logs] FTR Configs #42 / security APIs - Session Idle Session Idle cleanup should properly clean up session expired because of idle timeout when providers override global session config

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	5930	5931	+1
`securitySolutionServerless`	143	144	+1
total			+2

Async chunks

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

id	before	after	diff
`securitySolution`	20.5MB	20.5MB	+600.0B
`securitySolutionServerless`	119.2KB	132.6KB	+13.5KB
total			+14.1KB

Page load bundle

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

id	before	after	diff
`securitySolutionServerless`	21.6KB	21.9KB	+288.0B

Unknown metric groups

async chunk count

id	before	after	diff
`securitySolutionServerless`	14	15	+1

History

💛 Build #237925 was flaky 8b2e54d
💛 Build #237224 was flaky a6d764d
💚 Build #237030 succeeded dc36d81
💔 Build #236984 failed 5e110ce

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

cc @szwarckonrad

kibanamachine · 2024-10-01T13:20:46Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11126225795

This PR implements tier-based gating for custom notification messages in Protections. Only users on the Endpoint Complete tier will have the ability to modify these messages, while users on the Endpoint Essentials tier will no longer have this capability. If a user on the Essentials tier had made any changes to custom notifications before this update, those messages will be reset to the default ones. The changes are applied in three areas: 1. UI - An upsell banner is displayed for Essentials users. 2. API - We now prevent API calls that attempt to set or modify custom notification messages for Essentials users. 3. Policy Watcher - Upon Kibana startup (e.g., after a downgrade), we validate all policies for tier compliance. If a policy contains a custom notification message and the user is on the Essentials tier, the message will be reset to the default. ![Screenshot 2024-09-20 at 14 32 52](https://github.com/user-attachments/assets/75739338-e32b-47da-934e-9948f44099ae) ![Screenshot 2024-09-20 at 14 33 21](https://github.com/user-attachments/assets/1af081eb-f75f-4c9d-8f01-df9a01f8f2b2) ![Screenshot 2024-09-20 at 14 33 40](https://github.com/user-attachments/assets/4c0014f5-89f0-48b6-88dc-cc4c2dba666a) ![Screenshot 2024-09-20 at 14 52 25](https://github.com/user-attachments/assets/202e5e1a-7c58-4af1-a85a-399c94313f0b) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit af1dc87)

kibanamachine · 2024-10-01T13:25:05Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.x`: - [[EDR Workflows][Serverless] Gate custom note (#193171)](#193171)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Konrad Szwarc <konrad.szwarc@elastic.co>

UI

2f9dfda

szwarckonrad added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.16.0 labels Sep 17, 2024

szwarckonrad self-assigned this Sep 17, 2024

szwarckonrad and others added 11 commits September 19, 2024 15:53

Merge branch 'refs/heads/main' into gate-custom-note

6ce0ffa

API

fe468f2

Merge branch 'main' into gate-custom-note

c2e7b86

Clean up

67dfb85

Merge remote-tracking branch 'origin/gate-custom-note' into gate-cust…

34feeb3

…om-note

[CI] Auto-commit changed files from 'yarn openapi:bundle'

8b1e64c

Merge branch 'main' into gate-custom-note

1d8b86f

Merge branch 'refs/heads/main' into gate-custom-note

a48dd44

fix api validation error

d104abb

Merge branch 'refs/heads/main' into gate-custom-note

6d333c5

revert fix api validation error

ca2c1fb

szwarckonrad added v9.0.0 backport:version Backport to applied version labels labels Sep 20, 2024

szwarckonrad commented Sep 23, 2024

View reviewed changes

szwarckonrad marked this pull request as ready for review September 23, 2024 08:57

szwarckonrad requested review from a team as code owners September 23, 2024 08:57

szwarckonrad requested review from a team, paul-tavares and pzl September 23, 2024 08:57

szwarckonrad requested review from ashokaditya and removed request for pzl September 23, 2024 08:58

Merge branch 'main' into gate-custom-note

16d3215

semd approved these changes Sep 23, 2024

View reviewed changes

szwarckonrad added 3 commits September 25, 2024 14:02

Merge branch 'main' into gate-custom-note

7d41e6b

Copy change

5e110ce

Merge branch 'main' into gate-custom-note

dc36d81

paul-tavares approved these changes Sep 25, 2024

View reviewed changes

szwarckonrad added the ci:project-deploy-security Create a Security Serverless Project label Sep 26, 2024

Merge branch 'main' into gate-custom-note

a6d764d

ashokaditya approved these changes Sep 26, 2024

View reviewed changes

szwarckonrad added 3 commits September 30, 2024 10:48

Merge branch 'main' into gate-custom-note

9d18a61

# Conflicts: # x-pack/plugins/security_solution/server/endpoint/migrations/turn_off_policy_protections.test.ts # x-pack/plugins/security_solution/server/endpoint/migrations/turn_off_policy_protections.ts

merge + cr

8b2e54d

Merge branch 'main' into gate-custom-note

48ce7f6

szwarckonrad enabled auto-merge (squash) October 1, 2024 11:33

szwarckonrad merged commit af1dc87 into elastic:main Oct 1, 2024

kibanamachine mentioned this pull request Oct 1, 2024

[8.x] [EDR Workflows][Serverless] Gate custom note (#193171) #194576

Merged

Conversation

szwarckonrad commented Sep 17, 2024 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 23, 2024

Uh oh!

semd left a comment

Choose a reason for hiding this comment

Uh oh!

paul-tavares left a comment

Choose a reason for hiding this comment

Uh oh!

ashokaditya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

Uh oh!

kibanamachine commented Oct 1, 2024

Uh oh!

kibanamachine commented Oct 1, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

szwarckonrad commented Sep 17, 2024 •

edited by kibanamachine

Loading

kibana-ci commented Oct 1, 2024 •

edited

Loading