[Security Solution] UI Blocklist RBAC by gergoabraham · Pull Request #146455 · elastic/kibana

gergoabraham · 2022-11-28T18:08:27Z

Summary

Similarly to #145593 and #146111, this PR handles the None and Read privileges for the Blocklist sub-feature. The All privilege should not need any UI modification, but will need API modification.

The modification should:

hide Blocklist from Manage navigation items if privilege is NONE,
disable add/edit/delete for Blocklist if privilege is READ.
disable opening Policies from Blocklist (and any other ArtifactListPage) by disabling the links in the 'Applied for N policies' context menu

For testing the last part:

add Read privilege for Blocklist (or any other artifact using ArtifactListPage), and None to Policies
for now, it has to be tested with Fleet:All and Integrations:Read privileges

With Policies:Read privilege, hovering on the last item:

With Policies:None privilege, hovering on the last item:

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

elasticmachine · 2022-12-01T13:06:14Z

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

dasansol92

LGTM! Left minor suggestions on test cases but other than that, it looks great!

dasansol92 · 2022-12-01T13:56:34Z

x-pack/plugins/security_solution/public/management/links.test.ts

    expect(filteredLinks).toEqual(getLinksWithout(SecurityPageName.eventFilters));
  });

+  it('should hide Blocklist for user without privilege', async () => {


Should we add a test case for the case it has right privileges and Blocklist is shown?

There is a test case for showing all links, I believe that should be enough. What was missing is the correct name, so I renamed it: 00e8553

dasansol92 · 2022-12-01T13:57:18Z

x-pack/plugins/security_solution/public/management/pages/blocklist/view/blocklist.test.tsx

+      });
+    });
+
+    describe('READ privilege', () => {


Should we add a NONE privileges case?

This page is never displayed with NONE privilege - instead the NoPrivileges page is displayed, so I think there is no need for that test case.

paul-tavares

Left some feedback. Let me know your thoughts on my suggestion for the policy names in the card's popover

paul-tavares · 2022-12-05T14:39:28Z

...curity_solution/public/management/components/artifact_entry_card/components/effect_scope.tsx

            {children}
          </EuiButtonEmpty>
        }
+        disabled={!canReadPolicies}


I don't think its a good idea to display this with a disabled Button. When the user is not allowed to Read policies, then we should just display the policy name using regular text (<EuiText />). It looks strange (to me) for those entries to be displayed grey'ed out in the UI.

Thoughts?

I agree, so with some more effort, here is a new version: babbcf2

(I've added screenshots to the description)

paul-tavares · 2022-12-05T14:41:30Z

x-pack/plugins/security_solution/public/management/pages/blocklist/view/blocklist.test.tsx

      history.push(BLOCKLIST_PATH);
    });
+
+    mockedEndpointPrivileges = { canWriteBlocklist: true };


Can you use getEndpointAuthzInitialStateMock() here to instead of setting this to a partial object? it will be easier to maintain in the future, especially if we start to check other RBAC properties for whatever reason - they will be already set.

Good point, thanks!
b53c545

paul-tavares · 2022-12-06T14:55:19Z

...c/management/components/context_menu_with_router_support/context_menu_item_nav_by_router.tsx

 `;

+const StyledEuiText = styled(EuiText)`
+  padding: 10px 12px;


can you use Eui Style props here instead of hard coding it?

Should be able to use one of the "size" props in the theme - https://gist.github.com/paul-tavares/303d718ac7f514d6ee681a3851407a7e#file-eui_theme_style_props-js-L576-L579

(FYI: that's my own gist that I keep as a reference to what's available in the theme)

Thanks a lot for this suggestion! Unfortunately, I forgot about using theme constants, so I'm glad it came up.

1057e99

paul-tavares · 2022-12-06T14:57:54Z

x-pack/plugins/security_solution/public/management/links.test.ts

    });

-    it('should hide Event Filters for user without privilege', async () => {
+    it('should hide Blocklist for user without privilege', async () => {


Why did you change this test from Event filters to Blocklist?

It must be some merge/diff magic. 🤷

Originally I had should show and should hide tests for Trusted Apps, but then figured that there's only need for one should show all test, and individual should hide tests for all artefacts. So I renamed the should show Trusted Apps test and moved it up in this commit: 00e8553

Then got some merge conflicts and stuff, so here we are, with a messed up diff, where should show Trusted Apps became should hide Event filters, and should hide Event filters became should hide Blocklists. So I think all should be fine, every test is here.

paul-tavares

👍

gergoabraham · 2022-12-08T18:01:09Z

@elasticmachine merge upstream

gergoabraham · 2022-12-11T19:02:36Z

@elasticmachine merge upstream

kibana-ci · 2022-12-11T20:31:38Z

💚 Build Succeeded

Buildkite Build
Commit: 03b67c9

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`	10.1MB	10.1MB	+1.7KB

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	60	66	+6
`osquery`	109	115	+6
`securitySolution`	445	451	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	69	75	+6
`osquery`	110	117	+7
`securitySolution`	521	527	+6
total			+21

History

💔 Build #93699 failed f99ac76
💔 Build #93560 failed 0544715
💔 Build #93037 failed babbcf2
💔 Build #92675 failed 00e8553
💛 Build #92556 was flaky 9ce2e0f

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

cc @gergoabraham

gergoabraham added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.7.0 labels Nov 28, 2022

gergoabraham self-assigned this Nov 28, 2022

gergoabraham added 2 commits December 1, 2022 13:46

hide Blocklist from Management nav

8653afe

disable add/edit/delete for Blocklist for READ privilege

479cd6b

gergoabraham force-pushed the feat/olm-4918-ui-blocklist-ui-rbac branch from 1abd7a3 to 479cd6b Compare December 1, 2022 12:48

gergoabraham marked this pull request as ready for review December 1, 2022 13:06

gergoabraham requested a review from a team as a code owner December 1, 2022 13:06

gergoabraham requested review from dasansol92 and pzl December 1, 2022 13:06

dasansol92 approved these changes Dec 1, 2022

View reviewed changes

gergoabraham added 3 commits December 5, 2022 09:56

Merge branch 'main' into feat/olm-4918-ui-blocklist-ui-rbac

9ce2e0f

disable Policy links on ArtifactListPages if user has no privilege

7778f00

rename test case

00e8553

gergoabraham requested a review from paul-tavares December 5, 2022 13:59

paul-tavares reviewed Dec 5, 2022

View reviewed changes

use existing endpoint authz mock for tests

b53c545

pzl approved these changes Dec 6, 2022

View reviewed changes

use simple text for Policies instead of disabled buttons

babbcf2

paul-tavares reviewed Dec 6, 2022

View reviewed changes

gergoabraham added 4 commits December 8, 2022 13:06

use Eui sizes instead of hardcoded values

1057e99

Merge branch 'main' into feat/olm-4918-ui-blocklist-ui-rbac

0544715

fix failing ArtifactEntryCard test

e89a2f5

add new testcase for ArtifactEntryCard tests

acef078

gergoabraham requested a review from paul-tavares December 8, 2022 14:12

paul-tavares approved these changes Dec 8, 2022

View reviewed changes

gergoabraham enabled auto-merge (squash) December 8, 2022 17:10

Merge branch 'main' into feat/olm-4918-ui-blocklist-ui-rbac

f99ac76

Merge branch 'main' into feat/olm-4918-ui-blocklist-ui-rbac

03b67c9

gergoabraham merged commit b214406 into elastic:main Dec 11, 2022

gergoabraham deleted the feat/olm-4918-ui-blocklist-ui-rbac branch December 12, 2022 11:39

Conversation

gergoabraham commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Dec 1, 2022

Uh oh!

dasansol92 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!

paul-tavares 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!

paul-tavares left a comment

Choose a reason for hiding this comment

Uh oh!

gergoabraham commented Dec 8, 2022

Uh oh!

gergoabraham commented Dec 11, 2022

Uh oh!

kibana-ci commented Dec 11, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gergoabraham commented Nov 28, 2022 •

edited

Loading