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

[Kibana] Add a separate privilege under "Management" for allow/disallow "Saved Query" #158173

Closed
ninoslavmiskovic opened this issue May 22, 2023 · 21 comments · Fixed by #166937
Closed
Assignees
Labels
enhancement New value added to drive a business result impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.

Comments

@ninoslavmiskovic
Copy link
Contributor

Describe the feature:

Add a separate privilege under "Management" for allowing admins to control which users are able to utilize "Saved Queries"

Skærmbillede 2023-05-22 kl  13 55 12

Describe a specific use case for the feature:

If you have a user that has "read" only access to the Dashboard, but you want the user to be able to utilize the "Saved Query" feature, that is not possible today. Having "Saved Query" as a separate privilege, the admin will be able to set up a role that can utilize "Saved Query" on a Dashboard, even though the user only has "read" privilege for the Dashboard app.

@ninoslavmiskovic ninoslavmiskovic added the Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. label May 22, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@kertal kertal added enhancement New value added to drive a business result loe:large Large Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels May 23, 2023
@Danouchka Danouchka added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Jun 12, 2023
@Danouchka
Copy link

Hi @ninoslavmiskovic
Customer is willing to have this one released 1st.
it becomes critical for their operations.
Would you be open for a customer call so that they can show on their own dashboards and data but explain the impacts of not having this ?
Also do you have an approximate date of when that would be available (Plan of intent) ?

@ninoslavmiskovic
Copy link
Contributor Author

@Danouchka We had the call - Please share it internally.

@Danouchka
Copy link

Sure !

@kertal
Copy link
Member

kertal commented Aug 1, 2023

Adding the blocked label there, because we need security migrations to implement this feature, there's no way currently to implement it without a breaking change for existing users and their roles #68814

@kertal kertal added the blocked label Aug 1, 2023
@mwtyang
Copy link

mwtyang commented Sep 6, 2023

cc @arisonl @legrego

@arisonl
Copy link
Contributor

arisonl commented Sep 7, 2023

"Security migrations" is a substantial piece of work and this is why it has not been picked up until now (we were unsure whether there is enough justification for the effort). We will revive this conversation internally and circle back.

cc @mwtyang @ninoslavmiskovic

@arisonl
Copy link
Contributor

arisonl commented Sep 7, 2023

@ninoslavmiskovic Quick question: In the wireframe you attached, why is this not a sub-privilege under dashboards (and Discover -I remember they wanted it in Discover as well?) but rather under Management?

@kertal Is it a breaking change both as a sub-feature and alternatively as a separate feature privilege?

cc @mwtyang

@kertal
Copy link
Member

kertal commented Sep 8, 2023

@arisonl as a sub privilege it's a smaller breaking change, e.g. for rules like this, because the addition of a sub privilege will disallow this sub privilege for users with this role by default (given the role has "Customize sub privileges" on)
Bildschirmfoto 2023-09-08 um 09 52 34

I'm very aware that security migrations is a lot of work, but given the fact that we just added significant changes when switching between majors. But when there are no servers involved and the upgrade is more frequently, I guess any security rule add-on is blocked in the future?

@ninoslavmiskovic
Copy link
Contributor Author

As I see it the impact and the risk are lowest with continuing with adding 2 x sub privileges for Dashboard and Discover since it is only affecting users that have turned on the "Customize sub-feature privileges"

Therefore it is a smaller breaking change than adding it as a privilege of its own.

For the future migration effort, I recommend we still prioritize migration since it will help us in several cases, and tackle any challenges that are related to migration when we start developing it.

WDYT: @timductive , @mwtyang , @kertal , @arisonl

@Danouchka
Copy link

Hi

I remind the functional need:
be able to save 'saved queries' (I think loading is already ok) from read-only Dashboards
be able to save 'saved queries' (I think loading is already ok) from a read-only Discover

This way, a user can go back and forth from discover to dashboards loading, saving his saved queries

Thank you very much, Dan

@arisonl
Copy link
Contributor

arisonl commented Sep 8, 2023

We have no doubt that this is useful. We will discuss how this stacks up with the priority big items (mainly serverless) of the security team on Monday with @mwtyang and on Tuesday with the team.

On a high level, the problem is that if we split a set of privilege definitions to subsets, we do not currently have a way to map the privileges that a role is given from the previous situation to the new one. Without this piece of logic, we would break users currently assigned privileges and this piece of logic needs to be airtight and ideally to not require admins' actions.

@davismcphee
Copy link
Contributor

I want to point out that by introducing saved query only as a sub privilege of Dashboard and Discover, we would not be addressing the following areas (not an exhaustive list) which also allow users to create/load saved queries. We would need to introduce dedicated sub privileges for every single one of them in order to provide similar functionality:

  • Lens
  • Maps
  • TSVB
  • Vega
  • Aggregation based editor
  • Security Timelines
  • Security Alerts
  • Elasticsearch query rule
  • ...

Another thing to consider is that if we introduce saved query sub privileges now, we won't be able to migrate from many sub privileges to a single top-level privilege later once security migrations exists without introducing another breaking change, since there wouldn't be a way to reliably map from any combination of sub privileges to a single top-level privilege.

@arisonl
Copy link
Contributor

arisonl commented Sep 9, 2023

I understand that and this is your decision, our Analytics friends (our team will deliver the backend), but: first not all of these objects are a priority, e.g. TSVB is not, is Vega? Timelines don't even appear having separate controls I think (but that may be due to the fact that they cannot easily split them). My three cents:

  • It looks to me that we end up having a long list of heterogeneous things under "management" because basically we don't know where else to put them?
  • If there is an applicable MVP approach (e.g. dash and discover first like the initial ER asks for), we should go for it (I understand that this is not a straight line due to... well security being security, and an MVP may require to think about "everything" now)
  • I am hoping that with the security migrations we'll be able to allow you to move things without breaking them both directions, i.e. be able to resolve that missing mapping up or down level (TBC by our engineering)

@Danouchka
Copy link

@davismcphee

There's a real and practical problem of data governance and usage by hundreds customer operators that must be addressed in read-only Discover and Dashboards.... What you say is very wise indeed. However, we should not try to address everything and every hypothetical use cases that were not required by customers yet. (since Dashboards are read only, they wont be allowed to create visualizations with Lens, TSVB, maps ....etc)
And the "saved queries" sub privileges for this ER should be initialized with the parent privilege current permissions.

@Danouchka
Copy link

Danouchka commented Sep 12, 2023

Question: if the KQL bar is always accessible in Discover and Dashboards and that people can input any search, why not allowing them to always save and reload their saved queries ? This way, no sub privilege to code. What would be your take on that @ninoslavmiskovic @arisonl @davismcphee ?

I am going to test customer as well on that idea to see if it makes sense for them.

@Danouchka
Copy link

Customer is favorable to the idea of having saved queries always on , ie without the need of controlling it necessarily via a sub privilege.

@davismcphee
Copy link
Contributor

Hey @Danouchka, just acknowledging that I saw your recent comments. We've been discussing this internally and should be able to provide an update here soon.

@kertal kertal removed the blocked label Sep 14, 2023
@kertal
Copy link
Member

kertal commented Sep 14, 2023

I was removing the blocked label, because we figured out a way forward without a breaking changes. The way it works is pretty close like mentioned in the issue description. We won't introduce a sub privilege, but a dedicated global privilege for having saved queries "always on", while when this is not set, we keep the handling of the saved query privilege like it was before, on plugin level.

@mwtyang
Copy link

mwtyang commented Sep 14, 2023

@ninoslavmiskovic
Copy link
Contributor Author

Thank you for pulling this together @kertal and @davismcphee . 👏

jughosta added a commit that referenced this issue Sep 29, 2023
…es across Kibana (#166937)

- Resolves #158173

Based on PoC #166260

## Summary

This PR adds a new "Saved Query Management" privilege with 2 options:
- `All` will override any per app privilege and will allow users to save
queries from any Kibana page
- `None` will default to per app privileges (backward-compatible option)

<img width="600" alt="Screenshot 2023-09-21 at 15 26 25"
src="https://github.com/elastic/kibana/assets/1415710/6d53548e-5c5a-4d6d-a86a-1e639cb77202">

### 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

---------

Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
8 participants