-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Cases] Prevent fetching connectors if the user does not have read access to the actions plugin #127872
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
f75bc25
to
87402b4
Compare
@@ -0,0 +1,109 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it from x-pack/plugins/cases/public/containers/configure/mock.ts
to a new common file.
@elasticmachine merge upstream |
crud: !!casesCapabilities?.crud_cases, | ||
read: !!casesCapabilities?.read_cases, | ||
}, | ||
visualize: { crud: !!capabilities.visualize?.save, read: !!capabilities.visualize?.show }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of proxying values like this. Because we can simply put this code in the place we need it and that's it. That's why I kept visualize as a single boolean value that checks both conditions instead of separate ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are scenarios in which you need to know if the user has read
or crud
access and not both. Also, as a consumer of the hook if I have a single boolean I am not sure what kind of permissions the user has. I have to look at the code of the hook. By having separate properties I can immediately understand what is going on, it leaves no ambiguity and avoids security mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we can use the kibana capabilities object directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each plugin has a different name to define crud
or read
operations. As a developer, I need to know every time what this property is called and what is about. Also, the capabilities object is untyped. Having the hook add typing and there is no need to know the names. It will always be crud
and read
. Also, some of them have more than two capabilities. For example, the actions plugin has read
, execute
, delete
, save
. Surprisingly read
, execute
are both considered as read
. A new developer may make a mistake and use the wrong one. For all that reasons, I think is fine having this hook that a) force types b) export the same contract c) centralize all cases dependencies regarding capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware the capabilities was totally untyped. that's a good point. I see your point then as using this for standardizing them.
// TODO remove this check after the lens plugin fixes this bug | ||
appCapabilities.dashboard | ||
appCapabilities?.dashboard.crud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this #127963 merged we can remove this additional dashboard check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test changes!
My only comment is about proxying the capabilities object. I see more problematic that we take the existing values from kibana capabilities, put them in a custom object and re-export the same attributes with a different name. e.g. crud instead of save.
I rather we either use kibana capabilities directly or we actually enhance the capabilities.
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @cnasikas |
Summary
Fixes: #126784
Screenshots
Checklist
Delete any items that are not applicable to this PR.
For maintainers