[EDR Workflows] Fix Endpoint list RBAC problems #199803

gergoabraham · 2024-11-12T14:35:27Z

Summary

This PR fixes multiple Endpoint list privilege issues. It can be reviewed commit-by-commit so the fixes are mostly separated (although some solutions and tests are reused, hence the reason to have them in one pr):

a3311ad fixes issue when during onboarding (no hosts, policies are indiferent) calls are made to GET api/fleet/package_policies without correct privilege (needs policy management READ or fleet:READ+integration:READ), and causes Forbidden page. (issue)
UI: we display the usual 'onboarding without correct privileges' UI for users

63ca011 fixes issue when during onboarding (no hosts, no policies) the Add Elastic Defend button was shown when user had Fleet:ALL and Integrations:READ privilege, while both should be ALL in order to be able to create an integration policy (issue)
UI: the 'Add Elastic Defend' button is hidden, so the result is the same as above

Screen.Recording.2024-12-04.at.15.39.56.mov

ffafa14 fixes issue when after having hosts in Endpoint list and we're calling POST api/fleet/package_policies/_bulk_get without privilege (needs policy management READ or fleet:READ+integration:READ), which does not cause any visible issue, but is logged to dev console (issue)

some additions:

c7021b3 adds an acceptance test for all 3 issues above, with failing test run here.
8e10847 enables policy list integration test, this closes Failing test: Jest Integration Tests.x-pack/plugins/security_solution/public/management/pages/policy/view/integration_tests - When on the policy list page and data exists should show an avatar and name for the Updated by column #169133

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
Flaky Test Runner was used on any tests changed

closes elastic#169133

…ilege

gergoabraham · 2024-11-22T19:00:43Z

💔 Build Failed

Buildkite Build

Commit: c7021b3

Failed CI Steps

Defend Workflows Cypress Tests #13

Metrics [docs]

✅ unchanged

History

cc @gergoabraham

the test above is an acceptance test for desired behaviour, showing all 3 privilege issues in endpoint list which are fixed with this PR

gergoabraham · 2024-11-22T20:54:25Z

@elasticmachine merge upstream

gergoabraham · 2024-11-25T09:06:28Z

@elasticmachine merge upstream

elasticmachine · 2024-11-25T11:30:22Z

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

gergoabraham · 2024-11-27T10:22:49Z

@elasticmachine merge upstream

gergoabraham · 2024-11-27T14:13:43Z

@elasticmachine merge upstream

gergoabraham · 2024-12-02T09:28:16Z

@elasticmachine merge upstream

paul-tavares · 2024-12-03T14:12:53Z

hey @gergoabraham - thanks for working on this... could you post some screen captures of the UI for when the user has no authz to create integrations? especially around the views where changes were made.

paul-tavares

hi @gergoabraham - did a code review (only - did not run it locally) and left a few questions

x-pack/plugins/security_solution/common/endpoint/service/authz/authz.ts

x-pack/plugins/security_solution/common/endpoint/types/authz.ts

x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/store/middleware.ts

gergoabraham · 2024-12-04T12:19:12Z

@elasticmachine merge upstream

gergoabraham · 2024-12-04T14:52:25Z

hey @gergoabraham - thanks for working on this... could you post some screen captures of the UI for when the user has no authz to create integrations? especially around the views where changes were made.

added a screenshot and a video to the issue description 🎥

paul-tavares

Thanks for the changes and screen captures. 👍

paul-tavares · 2024-12-04T15:54:37Z

...curity_solution/public/management/cypress/e2e/endpoint_list/endpoints_rbac_mocked_data.cy.ts

+              cy.getByTestSubj('policyNameCellLink', { timeout: 3000 }).should('exist');
+              cy.getByTestSubj('policyNameCellLink', { timeout: 3000 }).within(() => {


Timeout seems low here (3s)... was this intentional?

thanks for catching that! it was not intentional to keep them. i shortened the timeouts at the start of implementation, so the test used as an acceptance test capturing the bugs fails faster on my machine and on CI, but now we don't need these ✅
99a2c18

szwarckonrad

Great work, code LGTM. Thanks for extensive test coverage :)

szwarckonrad · 2024-12-06T11:05:47Z

x-pack/plugins/security_solution/common/endpoint/service/authz/authz.test.ts

+        it(`should set canAccessFleet to ${value} if \`fleet.all\` is ${value}`, () => {
+          fleetAuthz.fleet.all = value;
+          expect(calculateEndpointAuthz(licenseService, fleetAuthz, userRoles).canAccessFleet).toBe(
+            value
+          );
+        });


Could this be a duplication of the one on line 100?

indeed it is! 🦅 👁️ !
9810335

szwarckonrad · 2024-12-06T11:15:19Z

x-pack/plugins/security_solution/common/endpoint/service/authz/authz.ts

+ * @param capabilities Capabilities from coreStart.application
+ */
+export const canFetchPackageAndAgentPolicies = (capabilities: Capabilities): boolean => {
+  const canReadPolicyManagement = (capabilities.siem?.readPolicyManagement ?? false) as boolean;


nit: const canReadPolicyManagement = Boolean(capabilities.siem?.readPolicyManagement);

here I see why using Boolean() helps, as we can get rid off the nullish coalescing operator, but...

95eb39f

szwarckonrad · 2024-12-06T11:17:05Z

x-pack/plugins/security_solution/common/endpoint/service/authz/authz.ts

+    (fleetv2?.agent_policies_read === true ||
+      fleetv2?.agent_policies_read === undefined)) as boolean;
+
+  const canReadIntegrations = capabilities.fleet?.read as boolean;


nit: const canReadIntegrations = Boolean(capabilities.fleet?.read);

here, i don't. i'm curious about your opinion: why do you prefer the Boolean() constructor to the as boolean type assertion here?

(changed anyway : ) 95eb39f)

szwarckonrad · 2024-12-06T11:18:32Z

x-pack/plugins/security_solution/common/endpoint/service/authz/authz.ts

+
+  const fleetv2 = capabilities.fleetv2;
+  const canReadFleetAgentPolicies = (fleetv2?.read &&
+    (fleetv2?.agent_policies_read === true ||


nit: A little bit hard to follow these conditions, wouldn't it come down to
(capabilities.fleetv2?.agent_policies_read ?? true) ?

yes, it comes to that. the idea was to be able to read it as we accept either true or undefined, but yeah, maybe simply a fallback to the true value is more readable
de27c61

elasticmachine · 2024-12-10T09:10:16Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: d3e7737

Failed CI Steps

:pipeline:🪄 Run Dynamic Pipeline: .buildkite/scripts/pipelines/pull_request/pipeline.sh

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`	14.7MB	14.7MB	+1.5KB

History

💔 Build #258281 failed 6fe4b3c
💛 Build #256922 was flaky ee762ef
💚 Build #255854 succeeded 0715246
💔 Build #254832 failed 72ec5cf
💚 Build #253870 succeeded a4c9984

cc @gergoabraham

gergoabraham · 2024-12-10T09:21:41Z

@elasticmachine merge upstream

elasticmachine · 2024-12-10T09:21:44Z

There are no new commits on the base branch.

kibanamachine · 2024-12-10T10:46:23Z

Starting backport for target branches: 8.x

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

## Summary This PR fixes multiple Endpoint list privilege issues. It can be reviewed commit-by-commit so the fixes are mostly separated (although some solutions and tests are reused, hence the reason to have them in one pr): - a3311ad fixes issue when during onboarding (no hosts, policies are indiferent) calls are made to `GET api/fleet/package_policies` without correct privilege (needs policy management READ or fleet:READ+integration:READ), and causes `Forbidden` page. ([issue](elastic/security-team#10581)) _UI_: we display the usual 'onboarding without correct privileges' UI for users <img width="1958" alt="image" src="https://github.com/user-attachments/assets/9e1701cc-9c3d-4a80-9c7a-df792d88dab3"> - 63ca011 fixes issue when during onboarding (no hosts, no policies) the `Add Elastic Defend` button was shown when user had `Fleet:ALL` and `Integrations:READ` privilege, while both should be `ALL` in order to be able to create an integration policy ([issue](elastic/security-team#10765)) _UI_: the 'Add Elastic Defend' button is hidden, so the result is the same as above https://github.com/user-attachments/assets/87fe3a95-131d-484b-8ca0-d06c4caafee1 - ffafa14 fixes issue when after having hosts in Endpoint list and we're calling `POST api/fleet/package_policies/_bulk_get` without privilege (needs policy management READ or fleet:READ+integration:READ), which does not cause any visible issue, but is logged to dev console ([issue](elastic/security-team#10580)) some additions: - c7021b3 adds an acceptance test for all 3 issues above, with failing test run [here](https://buildkite.com/elastic/kibana-pull-request/builds/250428#019320cf-c433-4979-a998-d0f8b8f7be16). - 8e10847 enables policy list integration test, this closes elastic#169133 ### 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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit 2fa8f47)

kibanamachine · 2024-12-10T10:51:37Z

💚 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] Fix Endpoint list RBAC problems (#199803)](#199803)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Gergő Ábrahám <[email protected]>

## Summary This PR fixes multiple Endpoint list privilege issues. It can be reviewed commit-by-commit so the fixes are mostly separated (although some solutions and tests are reused, hence the reason to have them in one pr): - a3311ad fixes issue when during onboarding (no hosts, policies are indiferent) calls are made to `GET api/fleet/package_policies` without correct privilege (needs policy management READ or fleet:READ+integration:READ), and causes `Forbidden` page. ([issue](elastic/security-team#10581)) _UI_: we display the usual 'onboarding without correct privileges' UI for users <img width="1958" alt="image" src="https://github.com/user-attachments/assets/9e1701cc-9c3d-4a80-9c7a-df792d88dab3"> - 63ca011 fixes issue when during onboarding (no hosts, no policies) the `Add Elastic Defend` button was shown when user had `Fleet:ALL` and `Integrations:READ` privilege, while both should be `ALL` in order to be able to create an integration policy ([issue](elastic/security-team#10765)) _UI_: the 'Add Elastic Defend' button is hidden, so the result is the same as above https://github.com/user-attachments/assets/87fe3a95-131d-484b-8ca0-d06c4caafee1 - ffafa14 fixes issue when after having hosts in Endpoint list and we're calling `POST api/fleet/package_policies/_bulk_get` without privilege (needs policy management READ or fleet:READ+integration:READ), which does not cause any visible issue, but is logged to dev console ([issue](elastic/security-team#10580)) some additions: - c7021b3 adds an acceptance test for all 3 issues above, with failing test run [here](https://buildkite.com/elastic/kibana-pull-request/builds/250428#019320cf-c433-4979-a998-d0f8b8f7be16). - 8e10847 enables policy list integration test, this closes elastic#169133 ### 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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed --------- Co-authored-by: Elastic Machine <[email protected]>

add acceptance test

c7021b3

gergoabraham added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution labels Nov 12, 2024

gergoabraham self-assigned this Nov 12, 2024

gergoabraham added 4 commits November 22, 2024 18:43

do not fetch agent policies without privileges - fixes "Forbidden" issue

a3311ad

fix and reenable policy list integration test

8e10847

closes elastic#169133

use toBeInTheDocument() in policy integration tests

47f9d94

do not show Endpoint onboarding without integration policy write priv…

63ca011

…ilege

gergoabraham force-pushed the fix-endpoint-list-rbac-problems branch from 7468e6c to 63ca011 Compare November 22, 2024 17:48

do not fetch package policies without needed privileges

ffafa14

Merge branch 'main' into fix-endpoint-list-rbac-problems

8ab0f05

Merge branch 'main' into fix-endpoint-list-rbac-problems

a4c9984

gergoabraham marked this pull request as ready for review November 25, 2024 11:30

gergoabraham requested a review from a team as a code owner November 25, 2024 11:30

gergoabraham requested review from ashokaditya and paul-tavares November 25, 2024 11:30

gergoabraham added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Nov 25, 2024

Merge branch 'main' into fix-endpoint-list-rbac-problems

72ec5cf

Merge branch 'main' into fix-endpoint-list-rbac-problems

c050ba4

Merge branch 'main' into fix-endpoint-list-rbac-problems

0715246

paul-tavares reviewed Dec 3, 2024

View reviewed changes

clarification for jsdoc

ee762ef

gergoabraham requested a review from paul-tavares December 4, 2024 14:52

paul-tavares approved these changes Dec 4, 2024

View reviewed changes

remove temporary testing timeouts

99a2c18

gergoabraham requested review from a team, pzl and szwarckonrad and removed request for ashokaditya and a team December 5, 2024 08:14

szwarckonrad approved these changes Dec 6, 2024

View reviewed changes

gergoabraham added 4 commits December 9, 2024 14:36

remove redundant test case

9810335

use Boolean() constructor

95eb39f

improve logic readability

de27c61

Merge branch 'main' into fix-endpoint-list-rbac-problems

6fe4b3c

gergoabraham enabled auto-merge (squash) December 9, 2024 14:39

Merge branch 'main' into fix-endpoint-list-rbac-problems

d3e7737

gergoabraham merged commit 2fa8f47 into elastic:main Dec 10, 2024
8 checks passed

kibanamachine added the v9.0.0 label Dec 10, 2024

kibanamachine mentioned this pull request Dec 10, 2024

[8.x] [EDR Workflows] Fix Endpoint list RBAC problems (#199803) #203534

Merged

kibanamachine added the v8.18.0 label Dec 10, 2024

gergoabraham deleted the fix-endpoint-list-rbac-problems branch December 10, 2024 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EDR Workflows] Fix Endpoint list RBAC problems #199803

[EDR Workflows] Fix Endpoint list RBAC problems #199803

gergoabraham commented Nov 12, 2024 •

edited by kibanamachine

Loading

gergoabraham commented Nov 22, 2024

💔 Build Failed

Failed CI Steps

Metrics [docs]

History

gergoabraham commented Nov 22, 2024

gergoabraham commented Nov 25, 2024

elasticmachine commented Nov 25, 2024

gergoabraham commented Nov 27, 2024

gergoabraham commented Nov 27, 2024

gergoabraham commented Dec 2, 2024

paul-tavares commented Dec 3, 2024

paul-tavares left a comment

gergoabraham commented Dec 4, 2024

gergoabraham commented Dec 4, 2024

paul-tavares left a comment

paul-tavares Dec 4, 2024

gergoabraham Dec 4, 2024

szwarckonrad left a comment

szwarckonrad Dec 6, 2024

gergoabraham Dec 9, 2024

szwarckonrad Dec 6, 2024

gergoabraham Dec 9, 2024

szwarckonrad Dec 6, 2024

gergoabraham Dec 9, 2024

szwarckonrad Dec 6, 2024

gergoabraham Dec 9, 2024

elasticmachine commented Dec 10, 2024 •

edited

Loading

gergoabraham commented Dec 10, 2024

elasticmachine commented Dec 10, 2024

kibanamachine commented Dec 10, 2024

kibanamachine commented Dec 10, 2024

		cy.getByTestSubj('policyNameCellLink', { timeout: 3000 }).should('exist');
		cy.getByTestSubj('policyNameCellLink', { timeout: 3000 }).within(() => {

[EDR Workflows] Fix Endpoint list RBAC problems #199803

[EDR Workflows] Fix Endpoint list RBAC problems #199803

Conversation

gergoabraham commented Nov 12, 2024 • edited by kibanamachine Loading

Summary

Checklist

gergoabraham commented Nov 22, 2024

💔 Build Failed

Failed CI Steps

Metrics [docs]

History

gergoabraham commented Nov 22, 2024

gergoabraham commented Nov 25, 2024

elasticmachine commented Nov 25, 2024

gergoabraham commented Nov 27, 2024

gergoabraham commented Nov 27, 2024

gergoabraham commented Dec 2, 2024

paul-tavares commented Dec 3, 2024

paul-tavares left a comment

Choose a reason for hiding this comment

gergoabraham commented Dec 4, 2024

gergoabraham commented Dec 4, 2024

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szwarckonrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 10, 2024 • edited Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

gergoabraham commented Dec 10, 2024

elasticmachine commented Dec 10, 2024

kibanamachine commented Dec 10, 2024

kibanamachine commented Dec 10, 2024

💚 All backports created successfully

Questions ?

gergoabraham commented Nov 12, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Dec 10, 2024 •

edited

Loading