[SecuritySolution][Detections] Fix "Closing a signal silently fails with reduced privileges"

[banderror]

Addresses: #56991

Summary

This PR introduces the following changes. If the user has insufficient write privileges on the signals index:

• we disable the status-changing actions on detection alerts ("Open alert", "Close Alert", "Mark in progress") in the context menu of an alert in alerts table
• we make sure to show the corresponding callout that tells about read-only access to detection alerts
• in the callout we provide links to docs for understanding why/how to fix

This is based on @spong's suggestion in the GH issue.

TODO:

• Add a new hasIndexUpdateDelete flag indicating true (full) write privilege on the signals index.
• Update ReadOnlyAlertsCallOut component to use hasIndexUpdateDelete instead of hasIndexWrite. This makes sure we show the callout.
• Update AlertContextMenu component to use hasIndexUpdateDelete. This makes sure we disable the context menu actions.
• Add links to docs to our read-only callouts: ReadOnlyAlertsCallOut and ReadOnlyRulesCallOut.

Screenshots

This is how it looks like for our readonly user role (the one that has "Read" on Kibana Security as well) - we show both callouts with the links (definitely not ideal, and this is another reason for improving/standardising callouts UI across the solution):

Details

Useful links

Some docs:

• Index privileges
• Kibana privileges
• I18n with <FormattedMessage />: doc 1, doc 2

Some related PRs:

• Xavier's change where he changes the implementation of hasIndexWrite
• Devin's comment on a very related case where the user clicks "Open alert", "Close Alert", "Mark in progress" and gets an error which is related to insufficient privileges (this is a different bug)

hasIndexWrite

This was an interesting one for me to play around and figure out how index privileges are implemented in Elasticsearch and in our security_solution codebase.

Docs for index privileges do not explicitly show how all of them are related to each other. Probably it's not a secret for many devs, but I had to experiment with assigning each privilege and checking which other privileges the user gets automatically:

POST /_security/user/_has_privileges
{
  "cluster": [
    "all"
  ],
  "index": [
    {
      "names": [".siem-signals*"],
      "privileges": [
        "all",
        "auto_configure",
        "create",
        "create_doc",
        "create_index",
        "delete",
        "delete_index",
        "index",
        "maintenance",
        "manage",
        "manage_follow_index",
        "manage_ilm",
        "manage_leader_index",
        "monitor",
        "read",
        "read_cross_cluster",
        "view_index_metadata",
        "write"
      ]
    }
  ]
}

E.g. I was assigning index, running this ES query and checking the result. If only index is granted, the user gets index, create and create_doc automatically. This is Elasticsearch behavoiur.

This way I figured out dependencies between all the index privileges (sorry for quality, looks like a kid's drawing):

What does this mean for us? If I'm not mistaken, this doesn't have to combine all of the privileges with OR:

hasIndexWrite:
  privilege.index[indexName].create ||
  privilege.index[indexName].create_doc ||
  privilege.index[indexName].index ||
  privilege.index[indexName].write,

because it folds to just privilege.index[indexName].create_doc, because write grants all of them automatically. And because of that hasIndexWrite actually makes sure that the user has create_doc 100%, but not necessarily create or index or write.

So perhaps it's a good idea to rename hasIndexWrite to hasIndexCreateDoc, because true hasIndexWrite (full write permissions guaranteeing updates and deletions) would be implemented like that:

hasIndexWrite: privilege.index[indexName].write,

I found a similar OR logic in lists plugin as well.

I18n

You can notice that callout messages are localized, but links to documentation are not. I did it on purpose to keep it simple and because I'm not sure they have to be localizable. The reason is - I'm not even sure we have docs for security solution in other languages. For me it seems like docs in other languages are outdated. Pls correct me if I'm wrong.

To do later?

I tried to keep this PR as thin as possible so it could be merged to 7.11 after feature freeze 🙂
But there are things that I was either considering doing right away (and then decided to postpone) or just thinking about.

• Some refactoring of the privileges-related code. I think it's a good idea to expose detailed privileges on every index and Kibana UI capabilities which is relevant to Security. Make it available across the app (common code) as SecuritySolutionUserPrivileges. Maybe create derived DetectionsUserPrivileges in detections like canUserUpdateAlerts. At least rename hasIndexWrite -> hasIndexCreateDoc and hasIndexUpdateDelete -> hasIndexWrite for consistency with Elasticsearch terminology.
• Add tooltips to disabled alert context menu items like in Devin's PR for rule actions.
• Should we better communicate the user about insufficient privileges to .lists* and .items*? Currently if the user doesn't have access to lists, we show an error message (toast) which you can see on the screenshots and in the issue. Should we create an error callout for that?
• Make a proper diagram showing dependencies between privileges and contribute to Elasticsearch docs.

I think there's some room for improvement here in how we implement RBAC in the UI: handle errors, communicate to the user what's possible and what's not, how we display it.

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[yctercero]

This looks great! I need to pull down to test, but wanted to just leave some initial thoughts after a quick glance.

Is it possible to make the callout an accordion. I know that's not currently available in EUI, so not sure if it's possible, but that would make things look a lot less busy I think. @marrasherrier might have more thoughts about showing multiple callouts on the page.

I see the error in the screenshots about not having read privileges on the .lists index. I need to dig in a bit more but I'm curious what functionality (other than lists) that could affect (this might be more of a separate issue to address). I mention it because in one we're using a callout, in the other a toast though both relate to permissions issues.

[yctercero]

Nice - I'm guilty of not following the docs and not using all caps when it's a function.

[banderror]

Thank you for review @yctercero !

Is it possible to make the callout an accordion. I know that's not currently available in EUI, so not sure if it's possible, but that would make things look a lot less busy I think. @marrasherrier might have more thoughts about showing multiple callouts on the page.

Yeah, that's similar to "Further rework towards cases-like implementation?" comment. I'll discuss with @peluja1012 and hopefully we'll plan it for some of the next releases. Accordion - didn't think about it, but cool idea!

I see the error in the screenshots about not having read privileges on the .lists index. I need to dig in a bit more but I'm curious what functionality (other than lists) that could affect (this might be more of a separate issue to address). I mention it because in one we're using a callout, in the other a toast though both relate to permissions issues.

Oh yes, that's a great comment. If there's some issue with privileges for .lists or .items, we show a toast with error. Which is not super descriptive and the user definitely can't do anything about it. I think we should build some unified way of checking permissions and communicating issues with them to the user. Probably based on callouts, but not necessarily. Maybe even common for the whole app, because the user uses the whole app rather than only detections tab. This one is also a separate topic and I'll chat on it with @peluja1012

[banderror]

@spong @yctercero @peluja1012 please re-check.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: e1e8f70

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
securitySolution	2160	2165	+5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
securitySolution	8.5MB	8.5MB	+4.4KB

Distributable file count

id	before	after	diff
default	47252	48012	+760

History

• 💚 Build #96986 succeeded d2afcac
• 💚 Build #96622 succeeded 1bc9a8980ac181fbd59ea06d10d747b6557f946e
• 💚 Build #96294 succeeded adeebbebf1828dcb32359251a8d4f2aaa458ac78
• 💚 Build #96196 succeeded 77d3865fdaff6ef97f82b5d2c0440221d8ee5f4b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[yctercero]

I think that there's already an ExternalLink component found here - x-pack/plugins/security_solution/public/common/components/links/index.tsx - we may want to make use of that one, or move this one into that file?

[banderror]

Thank you, this file seemed too busy for me and containing specific implementations like HostDetailsLinkComponent, so I overlooked ExternalLink there.

I'll check how it works and looks with the ExternalLink component from x-pack/plugins/security_solution/public/common/components/links/index.tsx and try to submit some quick improvement if possible.

[yctercero]

LGTM - some minor stuff that can be addressed in follow up. Definitely curious how we deal with privilege issues with other stuff like .lists and seeing if we can create a unified experience for it (as opposed to sometimes callouts, sometimes toasts, etc).

Thanks for this change!