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.

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] Fix Policy form being displayed as Read Only when displayed in Fleet pages #147212

Conversation

paul-tavares
Copy link
Contributor

@paul-tavares paul-tavares commented Dec 7, 2022

Summary

  • Fix Policy form being displayed as Read Only when displayed in Fleet pages

Tested:

With Role that DOES NOT have access to Security Solution

  • Fleet - Agent Policies - Edit agent Policy - Edit endpoint package policy edit form: 👍
  • Integrations Endpoint Page - Policies - edit form: 👍

With Role that does have policy management privilege to Security Solution

  • Policies - Edit policy form: 👍

@paul-tavares paul-tavares added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.7.0 labels Dec 7, 2022
@paul-tavares paul-tavares self-assigned this Dec 7, 2022
@paul-tavares paul-tavares marked this pull request as ready for review December 7, 2022 20:57
@paul-tavares paul-tavares requested review from a team as code owners December 7, 2022 20:57
@elasticmachine
Copy link
Contributor

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

@paul-tavares paul-tavares marked this pull request as draft December 7, 2022 22:47
@paul-tavares paul-tavares marked this pull request as ready for review December 12, 2022 23:06
@paul-tavares paul-tavares requested review from a team and ashokaditya and removed request for a team December 12, 2022 23:06
@kevinlog
Copy link
Contributor

checked it out and tried it, LGTM!

When superuser we can edit the forms in both Fleet and Security ✅
image

image

When Fleet ALL and Policy Mgmt none, we can still edit in Fleet ✅
image

When Policy Mgmt is ALL, we can edit in Security ✅
image

When Policy Mgmt is Read, we can only read in Security ✅
image

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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 -33.3KB

Page load bundle

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

id before after diff
securitySolution 50.5KB 50.5KB -35.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 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

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

cc @paul-tavares

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked it out and works as expected. I tested with the following RBAC privileges on a custom role.

  1. policy management:read, fleet:all, integrations:all/read. On the security side, I could only view the policy settings but not edit them. All form inputs/toggles were disabled and there was no Save button.

  2. policy management:all, fleet:all, integrations:all/read. On the security side, I could view and edit the policy settings. All input/toggles were enabled and I could see the Save button.

  3. policy management:none, fleet:all, integrations:all/read. On the security side, I could not see the policy link, and accessing the page with the URL shows me the privileges required callout.
    Screenshot 2022-12-14 at 10 51 54

  4. policy management:none, fleet:all, integrations:all. On the fleet side, I could view and edit the policy settings.

  5. policy management:none, fleet:all, integrations:read. On the fleet side, I could view but not edit the policy form. However, I could toggle some of the settings/inputs on the form, which is not the same as on the form on the security side where all form inputs/toggles are all disabled. Also clicking on cancel shows Discard Changes? confirm modal even where there are no changes to the form. I presume this is a bug on the fleet side and not related to changes here.

Here are screenshots to compare for 5.
security side (policy management:read, fleet:all, integrations:all/read)
Screenshot 2022-12-14 at 10 46 52

fleet side (policy management:none, fleet:all, integrations:read)
Screenshot 2022-12-14 at 10 46 38

Copy link
Contributor

@gergoabraham gergoabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate the small step-by-step commits, it's very easy to see the intention! 👏

});
});
afterEach(() => {
useUserPrivilegesMock.mockReturnValue(getUserPrivilegesMockDefaultValue());
});
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 should be in the beforeEach hook, because now the first test case passes accidentally, because it receives a mocked return value that's okay, but it comes from somewhere else, it's not defined in this file.

Actually I think it is weird that the first test passes - the return value of useUserPrivileges should be undefined until we configure a mock return value. But it looks like it is configured somewhere, but I couldn't find where. Do you have any idea? Does it maybe come from the depths of createFleetContextRendererMock()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gergoabraham ,

Yeah, some of our most used hooks are "auto-mocked" via Jest's __mocks__ directory. The useUserPrivileges() hook is one of those that you don't have to explicitly set on initial load - its mocked to set all privileges to true here:

https://github.com/elastic/kibana/blob/6efef0496077f4e61d49e4e43f75941ff3d98d9e/x-pack/plugins/security_solution/public/common/components/user_privileges/__mocks__/index.ts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, cool! That's good to know, thanks! And now I see that you are restoring exactly the same default implementation, great!

@paul-tavares paul-tavares merged commit ac6f2fb into elastic:main Dec 14, 2022
@paul-tavares paul-tavares deleted the task/olm-5610-fix-policy-form-shown-in-fleet branch December 14, 2022 14:36
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 14, 2022
* main: (21 commits)
  [Profiling] Remove link to 'Other' bucket (elastic#147523)
  [Synthetics UI] Add missing configuration options to the add/edit monitor forms (elastic#147265)
  [DOCS] Updates what's new pages (elastic#147483)
  [Fleet][Endpoint][RBAC V2] Update fleet router and config to allow API access via RBAC controls (elastic#145361)
  [Guided onboarding] Update guide IDs (elastic#147348)
  [Synthetics] Add synthetics settings alerting default (elastic#147339)
  [Security Solution][Endpoint] Fix Policy form being displayed as Read Only when displayed in Fleet pages (elastic#147212)
  [Cases] Save draft user comment (elastic#146327)
  [API Docs] Fix `--plugin` filter (elastic#147500)
  [Fleet] added a logic to use `destinationId` when tagging imported SOs (elastic#147439)
  Do not skip UPDATE_TARGET_MAPPINGS if upgrading to a newer stack version (elastic#147503)
  [Discover] Validate if Data View time field exists on Alert creation / editing (elastic#146324)
  [Discover] Fix Discover navigation from Lens embeddable (elastic#147000)
  Allow users to Update API Keys (elastic#146237)
  Update dependency xstate to ^4.35.0 (main) (elastic#147463)
  [Behavioral Analytics] Remove feature flag to hide functionality (elastic#147429)
  [Fleet] Add agent policy `inactivity_timeout`experimental setting (elastic#147432)
  [APM] Switching service groups from grid to flex layout (elastic#147448)
  [Fleet] Add missing endpoints to openApi specs (elastic#147452)
  [AO] Allow providing custom time range for Alert Summary Widget (elastic#147253)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants