Skip to content

[SecuritySolution] Check user permissions before initialising entity engine#198661

Merged
machadoum merged 11 commits intoelastic:mainfrom
machadoum:siem-ea-10926
Nov 6, 2024
Merged

[SecuritySolution] Check user permissions before initialising entity engine#198661
machadoum merged 11 commits intoelastic:mainfrom
machadoum:siem-ea-10926

Conversation

@machadoum
Copy link
Member

@machadoum machadoum commented Nov 1, 2024

Summary

  • Create privileges API for the Entity Store
  • Create missing privileges callout
  • Add missing Entity Store privileges callout to Entity Store
  • Add missing Entity Store privileges callout to Dashboard

Screenshot 2024-11-04 at 15 57 15
Screenshot 2024-11-04 at 16 16 03

Nov-04-2024.15-56-55.mp4

Update:

I added a "Line clamp" and "Read More" button as requested by Mark:
Screenshot 2024-11-05 at 13 15 51

Checklist

@machadoum machadoum self-assigned this Nov 4, 2024
@machadoum machadoum added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Theme: entity_analytics Feature:Entity Analytics Security Solution Entity Analytics features Team:Entity Analytics Security Entity Analytics Team v8.17.0 release_note:enhancement backport:version Backport to applied version labels labels Nov 4, 2024
@machadoum machadoum marked this pull request as ready for review November 4, 2024 15:16
@machadoum machadoum requested review from a team as code owners November 4, 2024 15:16
@machadoum machadoum requested a review from tiansivive November 4, 2024 15:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-entity-analytics (Team:Entity Analytics)

@machadoum machadoum changed the title WIP [SecuritySolution] Check user permissions before initialising entity engine Nov 4, 2024
@machadoum
Copy link
Member Author

Discussion Topic: I am checking privileges for all security solution indices. Is that the best approach?

@hop-dev
Copy link
Contributor

hop-dev commented Nov 4, 2024

@machadoum My initial feedback from just looking at the screenshots is that maybe we should not show all the missing privileges by default if there are a lot, we could have a "see more" to reveal all the permissions issues after the first 5 or so maybe?

@machadoum
Copy link
Member Author

machadoum commented Nov 5, 2024

@machadoum My initial feedback from just looking at the screenshots is that maybe we should not show all the missing privileges by default if there are a lot, we could have a "see more" to reveal all the permissions issues after the first 5 or so maybe?

@hop-dev Is there another place in Kibana using this pattern? Or do you think we should introduce it?
We could create a generic component that works like this: https://jsfiddle.net/ExplosionPIlls/6sj4e/

I found one place using this pattern inside Security Solution. I will reuse it.

Copy link
Contributor

@tiansivive tiansivive left a comment

Choose a reason for hiding this comment

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

a few comments, no blockers but worth discussing a bit imo

data-test-subj={`callout-${id}`}
data-test-messages={`[${id}]`}
>
<LineClamp maxHeight="100%" lineClampHeight={4.4}>
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe extract these magic values?

Copy link
Contributor

Choose a reason for hiding this comment

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

or is this component copy pasta?

Copy link
Member Author

Choose a reason for hiding this comment

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

maxHeight is 100% because I don't wanna the container to have a scroll bar.
But lineClampHeight was trial and error. The LineClamp component doesn't work well with non-text content, so I used decimals to compensate.

I will add `lineClampHeight' to a constant and add comments.

type: boolean
manage_transform:
type: boolean
additionalProperties:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is outside the scope of this ticket but it's weird to me that we're using OpenAPI to type the privileges object, when we already have TS types for it on kibana side, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but from what I know, we can't reuse TS types inside OpenAPi schemas. 😞

@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 5, 2024

💔 Build Failed

Failed CI Steps

History

cc @machadoum

@machadoum machadoum requested a review from a team as a code owner November 5, 2024 14:46
Copy link
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

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

@elastic/security-threat-hunting-investigations code review looks good 🚀 .

Thanks.

@machadoum machadoum added v8.18.0 and removed v8.17.0 labels Nov 6, 2024
Copy link
Contributor

@tiansivive tiansivive left a comment

Choose a reason for hiding this comment

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

Thanks!
I'm approving this PR as I think we can afford to merge this as is.
However, I noticed some inconsistencies.

For example, if we dont have risk score privileges, we also dont have enough privileges for enabling the store. Yet, we can open the modal and the risk score toggle will still be enabled, whilst the entity store one will show the missing privileges callout.

@machadoum
Copy link
Member Author

@tiansivive Good call.

I create a follow-up issue to kick-start the discussion. https://github.com/elastic/security-team/issues/11065

@machadoum machadoum merged commit 0e3b83b into elastic:main Nov 6, 2024
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.18

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

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.18 The branch "8.18" does not exist

Manual backport

To create the backport manually run:

node scripts/backport --pr 198661

Questions ?

Please refer to the Backport tool documentation

@machadoum
Copy link
Member Author

💚 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

machadoum added a commit to machadoum/kibana that referenced this pull request Nov 6, 2024
…engine (elastic#198661)

## Summary

* Create privileges API for the Entity Store
* Create missing privileges callout
* Add missing Entity Store privileges callout to Entity Store
* Add missing Entity Store privileges callout to Dashboard

![Screenshot 2024-11-04 at 15 57
15](https://github.com/user-attachments/assets/ed013571-4f0d-4605-bd2a-faa5ad3ac3e6)
![Screenshot 2024-11-04 at 16 16
03](https://github.com/user-attachments/assets/4cf6cf7d-a8c1-4c96-8fd1-2bf8be9f785e)

https://github.com/user-attachments/assets/30cdb096-24cd-4a1c-a20b-abbbece865d7

### Update:

I added a "Line clamp" and "Read More" button as requested by Mark:
![Screenshot 2024-11-05 at 13 15
51](https://github.com/user-attachments/assets/42fbec93-e258-49af-8acc-ae18314be442)

### Checklist
- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [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
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

(cherry picked from commit 0e3b83b)
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 7, 2024
machadoum added a commit that referenced this pull request Nov 7, 2024
…ntity engine (#198661) (#199162)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[SecuritySolution] Check user permissions before initialising entity
engine (#198661)](#198661)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Pablo
Machado","email":"pablo.nevesmachado@elastic.co"},"sourceCommit":{"committedDate":"2024-11-06T10:23:30Z","message":"[SecuritySolution]
Check user permissions before initialising entity engine (#198661)\n\n##
Summary\r\n\r\n* Create privileges API for the Entity Store\r\n* Create
missing privileges callout\r\n* Add missing Entity Store privileges
callout to Entity Store \r\n* Add missing Entity Store privileges
callout to Dashboard\r\n\r\n![Screenshot 2024-11-04 at 15
57\r\n15](https://github.com/user-attachments/assets/ed013571-4f0d-4605-bd2a-faa5ad3ac3e6)\r\n![Screenshot
2024-11-04 at 16
16\r\n03](https://github.com/user-attachments/assets/4cf6cf7d-a8c1-4c96-8fd1-2bf8be9f785e)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/30cdb096-24cd-4a1c-a20b-abbbece865d7\r\n\r\n###
Update:\r\n\r\nI added a \"Line clamp\" and \"Read More\" button as
requested by Mark:\r\n![Screenshot 2024-11-05 at 13
15\r\n51](https://github.com/user-attachments/assets/42fbec93-e258-49af-8acc-ae18314be442)\r\n\r\n\r\n###
Checklist\r\n- [x] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] Any UI
touched in this PR is usable by keyboard only (learn more\r\nabout
[keyboard
accessibility](https://webaim.org/techniques/keyboard/))","sha":"0e3b83b595906b42fc386e19451759399ee3e74e","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","v9.0.0","Team:
SecuritySolution","Theme: entity_analytics","Feature:Entity
Analytics","Team:Entity
Analytics","backport:version","v8.18.0"],"number":198661,"url":"https://github.com/elastic/kibana/pull/198661","mergeCommit":{"message":"[SecuritySolution]
Check user permissions before initialising entity engine (#198661)\n\n##
Summary\r\n\r\n* Create privileges API for the Entity Store\r\n* Create
missing privileges callout\r\n* Add missing Entity Store privileges
callout to Entity Store \r\n* Add missing Entity Store privileges
callout to Dashboard\r\n\r\n![Screenshot 2024-11-04 at 15
57\r\n15](https://github.com/user-attachments/assets/ed013571-4f0d-4605-bd2a-faa5ad3ac3e6)\r\n![Screenshot
2024-11-04 at 16
16\r\n03](https://github.com/user-attachments/assets/4cf6cf7d-a8c1-4c96-8fd1-2bf8be9f785e)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/30cdb096-24cd-4a1c-a20b-abbbece865d7\r\n\r\n###
Update:\r\n\r\nI added a \"Line clamp\" and \"Read More\" button as
requested by Mark:\r\n![Screenshot 2024-11-05 at 13
15\r\n51](https://github.com/user-attachments/assets/42fbec93-e258-49af-8acc-ae18314be442)\r\n\r\n\r\n###
Checklist\r\n- [x] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] Any UI
touched in this PR is usable by keyboard only (learn more\r\nabout
[keyboard
accessibility](https://webaim.org/techniques/keyboard/))","sha":"0e3b83b595906b42fc386e19451759399ee3e74e"}},"sourceBranch":"main","suggestedTargetBranches":["8.18"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198661","number":198661,"mergeCommit":{"message":"[SecuritySolution]
Check user permissions before initialising entity engine (#198661)\n\n##
Summary\r\n\r\n* Create privileges API for the Entity Store\r\n* Create
missing privileges callout\r\n* Add missing Entity Store privileges
callout to Entity Store \r\n* Add missing Entity Store privileges
callout to Dashboard\r\n\r\n![Screenshot 2024-11-04 at 15
57\r\n15](https://github.com/user-attachments/assets/ed013571-4f0d-4605-bd2a-faa5ad3ac3e6)\r\n![Screenshot
2024-11-04 at 16
16\r\n03](https://github.com/user-attachments/assets/4cf6cf7d-a8c1-4c96-8fd1-2bf8be9f785e)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/30cdb096-24cd-4a1c-a20b-abbbece865d7\r\n\r\n###
Update:\r\n\r\nI added a \"Line clamp\" and \"Read More\" button as
requested by Mark:\r\n![Screenshot 2024-11-05 at 13
15\r\n51](https://github.com/user-attachments/assets/42fbec93-e258-49af-8acc-ae18314be442)\r\n\r\n\r\n###
Checklist\r\n- [x] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] Any UI
touched in this PR is usable by keyboard only (learn more\r\nabout
[keyboard
accessibility](https://webaim.org/techniques/keyboard/))","sha":"0e3b83b595906b42fc386e19451759399ee3e74e"}},{"branch":"8.18","label":"v8.18.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
@kibanamachine kibanamachine added v8.17.0 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:Entity Analytics Security Solution Entity Analytics features release_note:enhancement Team:Entity Analytics Security Entity Analytics Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Theme: entity_analytics v8.17.0 v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants