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

NS Integrations results provider: avoid throwing when the user does not have superuser permissions #111094

Closed
Tracked by #115784
pgayvallet opened this issue Sep 3, 2021 · 3 comments
Labels
bug Fixes for quality problems that affect the customer experience Feature:Navigational Search Global search bar Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@pgayvallet
Copy link
Contributor

In #110778 we discovered that when a plain user is using the navigational search, the integrations result provider throws an error, resulting on all the results not being displayed (effectively breaking the whole NS feature)

#111093 addressed it, by catching potential errors from providers to allow the results from other providers to be properly returned.

However, in theory, this shouldn't be necessary. a NS result provider is supposed to work correctly for any kind of user. Potential ACL checks must remains an implementation detail, and not surface by throwing errors from the result observable.

We need to fix the integrations result provider to not emit errors when the user is not a superuser, either by performing a client-side authz check, or by catching the potential 403 returned by the server.

Second thing is, this provider is performing an HTTP call to the backend.

It's stated in the plugin's readme that when possible, a server-side provider should be favored

Results from providers registered from the client-side registerResultProvider API will
not be available when performing a search from the server-side. For this reason, prefer
registering providers using the server-side API when possible.

Did we have any valid reason to implement a client-side provider instead of a server-side for integrations? From a quick look, it doesn't seem the case. If nothing blocks it, the provider should be migrated to the server-side.

@pgayvallet pgayvallet added bug Fixes for quality problems that affect the customer experience Team:Fleet Team label for Observability Data Collection Fleet team Feature:Navigational Search Global search bar labels Sep 3, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@joshdover
Copy link
Contributor

Did we have any valid reason to implement a client-side provider instead of a server-side for integrations? From a quick look, it doesn't seem the case. If nothing blocks it, the provider should be migrated to the server-side.

The current functionality could be implemented server-side, however we are planning to add some new items to the search that may require us to keep this client-side. I'll include investigating moving this server-side as part of our improvements in that area.

In the meantime, I agree we should not throw errors when the user is not superuser. The updates we're planning in #93084 also include no longer requiring superuser to access the UI these search results route to. If it appears that those changes are not going to make 7.16, we should at least fix this bug and just return empty results for non-superusers.

@criamico
Copy link
Contributor

@jen-huang I think that this issue can be closed because of the superuser removal work done in 8.0 - it's now handled through the new sets of privileges, so this should not be relevant anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Navigational Search Global search bar Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

No branches or pull requests

4 participants