-
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
[index management] Allow view only access to Index Templates, Component Templates and Enrich policies #195162
base: main
Are you sure you want to change the base?
[index management] Allow view only access to Index Templates, Component Templates and Enrich policies #195162
Conversation
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
cc @mattkime |
/ci |
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.
x-pack/test/functional/config.base.js
changes LGTM
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.
Awesome work @mattkime! Tested locally and all works well. I do however have something I'd like to discuss as part of these new changes:
The missing privileges callout we used to have was very concise copy wise and will tell exactly what privilege(s) the user was missing
Whereas right now the error we show to them it's not as friendly, gives information that might not be relevant to them (current user, action) and it doesn't also specifically say which privileges they are missing.. just which ones are required.
Is there anyway we can extract some of the information from the error and perhaps have a more user friendly copy?
@@ -79,7 +77,7 @@ const ListView: React.FunctionComponent<RouteComponentProps> = ({ history, locat | |||
return <ErrorState error={error} resendRequest={reloadPolicies} />; | |||
} | |||
|
|||
if (policies?.length === 0) { | |||
if (capabilities.index_management.manageEnrich && policies?.length === 0) { |
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.
With this change we lose the empty state if they dont have the right permissions, how about we remove the CTA in the empty state component instead?
...c/application/sections/home/template_list/legacy_templates/template_table/template_table.tsx
Show resolved
Hide resolved
</EuiButton>, | ||
]; | ||
|
||
if (capabilities.index_management.manageEnrich) { |
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.
Just a thought:
We are passing the capabilities object in quite a few places and accessing its internal keys in order to figure out if the user has the manageEnrich
privilege. I think having a computed property in the app context (something like hasManageEnrichPrivilege) would avoid us having to specify the full path everywhere and perhaps also be a little bit more semantic than capabilities.index_management.manageEnrich
. It might also simplify test setup as we would only need to mock a boolean instead of a full dependency.
Also, a bit unlikely I guess, but if this permission path were to change we would also have to replace it in a bunch of files rather than just once.
Summary
Previously, Index Templates, Component Templates and Enrich policies failed to provide view access. This has been addressed by removing endpoints that were used for edit privilege checks and replacing them capabilities api usage only around editing features.
Closes #178654