[Management] If "All" cluster privileges is selected, allowing unchecked other options is misleading

[elasticmachine]

Original comment by @Stacey-Gammon:

Unless I am missing something, the following seems to give the role security access:

But this doesn't:

Is there any reason, that selecting "All" doesn't automatically select all the other options, and if one of the sub areas is unchecked, then it would automatically uncheck "all"?

cc @skearns64 @cjcenizal @snide

[elasticmachine]

Original comment by @skearns64:

++ this is a bug. It's not a critical one, but it can lead to a misunderstanding of cluster privileges.

I'm happy with your suggested approach, but I prefer the behavior to be: "if all is checked, all the other cluster privilege checkboxes are grayed out". This simplifies the interaction a bit.

I marked this as low-fruit on the assumption that is true, anyone is welcome to remove that label if I've guessed wrong :)

[elasticmachine]

Pinging @elastic/kibana-security

[legrego]

Now that the cluster privileges are using a multi-select dropdown, the interaction is a bit different, but still misleading.

I think in general we would want a more comprehensive solution, since I believe that we have other cluster privileges that effectively grant other cluster privileges. Or to say another way, some privileges are subsets of other privileges, and we shouldn't only come up with a solution to the all case.

[legrego]

Since this issue was opened, we have started relying on the "builtin privileges API" to populate the available cluster and index privileges.

In order to make progress here, we would need to enhance the ES API to give us the relationship between each of these privileges. Kibana does not have enough information to make this determination on its own.

[legrego]

Since this issue was opened, we have started relying on the "builtin privileges API" to populate the available cluster and index privileges.

In order to make progress here, we would need to enhance the ES API to give us the relationship between each of these privileges. Kibana does not have enough information to make this determination on its own.

@elastic/es-security do you have thoughts on the technical feasibility of this? Essentially, this is asking for a privilege hierarchy for cluster (and I suppose index) privileges, so we know which privileges are subsets of others.

[ywangd]

On the ES side, to improve error message when access is denied, @tvernum added sorting for both index and cluster privileges (https://github.com/elastic/elasticsearch/pull/60357, https://github.com/elastic/elasticsearch/pull/66900) so that we can recommend privileges from least to most powerful.

The sorting is not perfect though. It is based on the number of privileges implied by a privilege, e.g. all implies all other privileges so that it is the most powerful. But because the actual heirarchy is not linear, it cannot really be represented by a list. For example, manage_api_key and manage_token have no real order between them. But in a list, you have to place them in order but the order does not really mean much. This imperfection is not an issue for our use case because those unrelated privileges do not simultaneously grant permission to the same (denied) action. But for Kibana's use case, it may be an issue?

The current GetBuiltinPrivileges API return privileges sorted by names. We can change (maybe subject to BWC) the sort order to be "least-to-most-powerful" as described above if this is helpful for Kibana.

Alternatively, it may also be possible to build full hierarchy of the privileges. A new API is probably needed to expose this result. This will be an overall larger effort.

[legrego]

Thanks, Yang. It sounds like what we would really need is the full hierarchy, but I don't think it's worth taking on at this point given that it's not a trivial amount of work.

[tvernum]

I don't think it's a huge technical effort. We know which privileges imply which other privileges, and we can find a way to return that in an API. But the existing API isn't really designed to be evolved that way.

I threw something together in the background (https://github.com/tvernum/elasticsearch/commit/b118aa329d7) that horribly abuses the current API by adding a format=tree option that returns something like:

    {
      "name": "manage",
      "implies": [
        "cancel_task",
        "create_snapshot",
        "manage_autoscaling",
        "manage_data_frame_transforms",
        "manage_enrich",
        "manage_ilm",
        "manage_index_templates",
        "manage_ingest_pipelines",
        "manage_logstash_pipelines",
        "manage_ml",
        "manage_pipeline",
        "manage_rollup",
        "manage_slm",
        "manage_transform",
        "manage_watcher",
        "monitor",
        "monitor_data_frame_transforms",
        "monitor_ml",
        "monitor_rollup",
        "monitor_snapshot",
        "monitor_text_structure",
        "monitor_transform",
        "monitor_watcher",
        "read_ilm",
        "read_pipeline",
        "read_slm",
        "transport_client"
      ]
    },
    {
      "name": "manage_api_key",
      "implies": [
        "grant_api_key",
        "manage_own_api_key"
      ]
    },
    {
      "name": "manage_autoscaling"
    },
    {
      "name": "manage_ccr",
      "implies": [
        "read_ccr"
      ]
    },

The problem isn't the amount of work (I hacked something together on a couple of hours while working on something else), it's coming up with a way to fit it into ES without just adding a whole new API.

[ywangd]

I don't think it's a huge technical effort.

I don't think it's huge either. I meant it is a "larger" effort, not "large" ("larger" than just sorting the existing returned values in GetPrivileges API that is).

it's coming up with a way to fit it into ES without just adding a whole new API.

That's where the majority of work would be. I suspect the Clients team would rather dislike the idea of having a query parameter to control the response format. I haven't asked them, but it feels something challenging with existing client generation framework. I think this is an idea worth exploring in general. I had similar thought when trying to come up a way for HasPrivileges to return "partial". It is a middle-ground between breaking changes and entirely new APIs. Our BWC guideline says:

• Adding a query parameter is not a breaking change
• Changing the response format is a break change

But did not say anything about whether it is a breaking change if we add the above two together. Some clarification on this topic would be very helpful.

[ywangd]

For this specific API, maybe we can get away with just adding a new top level field, say hierarchy, in the response. Currently the top level fields are cluster and index. They do not match up with hierarchy too nicely. But it is not too bad either.

[legrego]

Thanks Tim & Yang. It feels like adding a new hierarchy field would be a reasonable compromise given our constraints. @elastic/kibana-security do you have any thoughts on the response format, given the constraints that Tim and Yang mentioned above?

[azasypkin]

Thanks Tim & Yang. It feels like adding a new hierarchy field would be a reasonable compromise given our constraints. https://github.com/orgs/elastic/teams/kibana-security do you have any thoughts on the response format, given the constraints that Tim and Yang https://github.com/elastic/kibana/issues/18482#issuecomment-1214605569 https://github.com/elastic/kibana/issues/18482#issuecomment-1214614857?

I don't have a strong opinion to be honest, privilege's specific implies field feels more appropriate, but if it comes with a high cost (including more complex or "loosy" signatures in Elasticsearch clients) then a separate top-level hierarchy/explanation field sounds reasonable too.

[tvernum]

I don't have a strong opinion to be honest, privilege's specific implies field feels more appropriate

You could have something like:

{
  "cluster": [ ... ],
  "index": [ ... ],
  "implies": {
    "cluster": {
      "manage_api_key": [ "grant_api_key", "manage_own_api_key" ],
      "manage_autoscaling": [ ],
      "manage_ccr": [ "read_ccr" ]
     },
    "index": { ... }
}

I haven't attempted to work out how well that sits with the API guidelines. It's a bit of a band-aid though, because it solves this immediate need, but doesn't help prepare for any future changes.

[azasypkin]

You could have something like:

Yeah, I meant that having hierarchy or implies on the same level as different privileges "scopes" feels a bit weird (since it doesn't define another "scope", but rather clarifies other scopes), but it's not a big concern, I was just nitpicking.