[Security Solution] UI trusted applications RBAC by gergoabraham · Pull Request #145593 · elastic/kibana

gergoabraham · 2022-11-17T16:48:03Z

Summary

RBAC UI features for Trusted Applications. To test, enable endpointRbacEnabled feature-flag, create a non-superuser user with Security: ALL privilege and (All | Read | None) sub-privilege for Trusted Applications.

The modification should:

hide Trusted Apps from Manage navigation items if privilege is NONE, (note: it is still displayed for non-superusers, if the feature flag is disabled)
disable add/edit/delete for Trusted Applications if privilege is READ.

⚠️ Note

This PR focuses on Read and None. The sub-privilege All does not work perfectly at the moment, because of unauthorised API calls. A follow-up PR will fix this, after this PR is merged: #145361

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-11-21T14:05:30Z

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

ashokaditya

Mostly nits, but it's good to 🚢

ashokaditya · 2022-11-23T09:56:50Z

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

    linksToExclude.push(SecurityPageName.hostIsolationExceptions);
  }

+  if (endpointRbacEnabled && !canReadTrustedApplications) {


endpointRbacEnabled is redundant here, as canReadTrustedApplications is already calculated with isEndpointRbacEnabled which is true if either FF (endpointRbacEnabled, endpointRbacV1Enabled) is true.

There is a slight difference in the operation with and without the feature flag:

if RBAC is enabled, we should hide the Trusted Apps from nav,

if disabled, Trusted Apps should be visible to non-superusers, too, but they are shown the Privileges Required screen when clicking on it.

So this condition here is to ensure that the operation stays exactly the same if the feature flag is disabled.

@gergoabraham ,
I agree with @ashokaditya here. The fact that the TA menu items appear before, even if the user did not have permission to it, was actually not the desired behaviour and is something we been meaning to correct for a while.

@paul-tavares
Oh, I see, didn't know that it was a not desired behaviour. I'll clean it up in a next RBAC UI PR then. 👍 Another benefit is that there will be less to clean up when removing the feature flag.

It's here: 8ecd2ba

ashokaditya · 2022-11-23T10:01:34Z

...ugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_list.test.tsx

+    mockedEndpointPrivileges = { canWriteTrustedApplications: true };
+    mockUserPrivileges.mockReturnValue({ endpointPrivileges: mockedEndpointPrivileges });


Consider resetting mockUserPrivileges after each test

nice catch, thanks!

ashokaditya · 2022-11-23T10:08:52Z

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

+  const getLinksWithout = (...excludedLinks: SecurityPageName[]) => ({
+    ...links,
+    links: links.links?.filter((link) => !excludedLinks.includes(link.id)),
+  });
+


dasansol92

LGTM! Left minor suggestion but other than that, this is good to go!

dasansol92 · 2022-11-23T10:07:27Z

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

  let getPlugins: (roles: string[]) => StartPlugins;
  let fakeHttpServices: jest.Mocked<HttpSetup>;

+  const getLinksWithout = (...excludedLinks: SecurityPageName[]) => ({


Would be worth using the without function from lodash?

I wasn't familiar with without so thanks for the suggestion! However, it looks like that without uses equality comparison on objects, so I don't think it's possible to filter out objects based on their attributes, link.id here.

kibana-ci · 2022-11-23T12:22:50Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: dd367cc

Failed CI Steps

Security Solution Tests #1

Test Failures

[job] [logs] Security Solution Tests #1 / Cases Creates a new case with timeline and opens the timeline

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`	9.7MB	9.7MB	+196.0B

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`	59	65	+6
`osquery`	109	115	+6
`securitySolution`	443	449	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	68	74	+6
`osquery`	110	117	+7
`securitySolution`	520	526	+6
total			+21

History

💚 Build #90211 succeeded 0c09f1a
💚 Build #90079 succeeded 4814c22
💛 Build #89661 was flaky 763893a
💚 Build #89530 succeeded c20c08e
💔 Build #89128 failed bf8c768bde4024051a38e9243d9d25f2d64b5654
💔 Build #88908 failed 3cc4cc534928aec45a6ba5814dce7dc80445dcec

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

cc @gergoabraham

## Summary Similarly to #145593, this PR handles the _None_ and _Read_ privileges for the Event Filters sub-feature. The _All_ privilege should not need any UI modification, but will need API modification. <img width="554" alt="image" src="https://user-images.githubusercontent.com/39014407/203514418-b016a47b-819c-4057-a86e-d7b4a3d8e5c5.png"> The modification should: - hide Event Filters from Manage navigation items if privilege is NONE, ~(note: it is still displayed for non-superusers, if the feature flag is disabled)~ update: it is hidden for non-superusers if the feature flag is disabled - disable add/edit/delete for Event Filters if privilege is READ. #### Checked: - the Event Filters form still works from the "Hosts > Events" side of the app ✅ <img width="1354" alt="image" src="https://user-images.githubusercontent.com/39014407/204316619-85121106-9d28-4165-9675-522890e39dfe.png"> <img width="1323" alt="image" src="https://user-images.githubusercontent.com/39014407/204326904-6917c8fe-a364-4a40-8bdc-e8240115fa1d.png"> ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## 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. <img width="541" alt="image" src="https://user-images.githubusercontent.com/39014407/204349035-ca234eae-66ec-4018-bc04-8deaebdd8a0b.png"> 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: <img width="773" alt="image" src="https://user-images.githubusercontent.com/39014407/205944360-fed60b11-7a88-42d5-93cd-307c7b34891b.png"> With `Policies:None` privilege, hovering on the last item: <img width="778" alt="image" src="https://user-images.githubusercontent.com/39014407/205944198-7dccfa37-177f-4eb7-a773-09eaeaa4b1fe.png"> ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

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.6.0 labels Nov 17, 2022

gergoabraham self-assigned this Nov 17, 2022

gergoabraham force-pushed the feature/olm-4913-ui-trusted-applications-rbac branch from 3cc4cc5 to bf8c768 Compare November 18, 2022 09:34

gergoabraham added 2 commits November 21, 2022 09:19

hide Trusted Apps from manage nav

4ed8187

some refactor on links.test.ts

c20c08e

gergoabraham force-pushed the feature/olm-4913-ui-trusted-applications-rbac branch from bf8c768 to c20c08e Compare November 21, 2022 08:19

disable add/edit/delete for Trusted Applications for READ privilege

763893a

gergoabraham marked this pull request as ready for review November 21, 2022 14:05

gergoabraham requested a review from a team as a code owner November 21, 2022 14:05

gergoabraham requested review from ashokaditya and parkiino November 21, 2022 14:05

gergoabraham added v8.7.0 and removed v8.6.0 labels Nov 21, 2022

gergoabraham added 2 commits November 22, 2022 15:48

Merge branch 'main' into feature/olm-4913-ui-trusted-applications-rbac

4814c22

fix falsely passing unit test

0c09f1a

gergoabraham mentioned this pull request Nov 23, 2022

[Security Solution] UI Event Filters RBAC #146111

Merged

1 task

ashokaditya approved these changes Nov 23, 2022

View reviewed changes

dasansol92 approved these changes Nov 23, 2022

View reviewed changes

reset mock after each test case

dd367cc

gergoabraham merged commit 4c6a915 into elastic:main Nov 23, 2022

gergoabraham deleted the feature/olm-4913-ui-trusted-applications-rbac branch November 23, 2022 12:50

This was referenced Nov 23, 2022

[Security Solution RBAC: update API call permissions #145865

Closed

[Security Solution] UI Blocklist RBAC #146455

Merged

		mockedEndpointPrivileges = { canWriteTrustedApplications: true };
		mockUserPrivileges.mockReturnValue({ endpointPrivileges: mockedEndpointPrivileges });

Conversation

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

Uh oh!

Summary

⚠️ Note

Checklist

Uh oh!

elasticmachine commented Nov 21, 2022

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!

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!

kibana-ci commented Nov 23, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

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!

6 participants

gergoabraham commented Nov 17, 2022 •

edited

Loading