[Security Solution] [Detections] Only display actions options if user has "read" privileges

[dhurley14]

Summary

Fixes #74170

updates the detections privileges route to include checking for "read" privilege on actions. Also updates the rule create / edit steps to display a message if the user is missing the actions privileges while still allowing the user to create and activate a rule.

testing:

This script will post a role with the necessary detections permissions

curl -H 'Content-Type: application/json' -H 'kbn-xsrf: 123'\
 -u ${ELASTICSEARCH_USERNAME}:${ELASTICSEARCH_PASSWORD} \
-XPUT ${KIBANA_URL}/api/security/role/my_detections_role \
-d '{
  "elasticsearch": {
    "cluster": ["manage"],
    "indices": [
      {
        "names": ["auditbeat-*", "packetbeat-*", ".siem-signals-*", ".lists*", ".items*"],
        "privileges": ["manage", "write", "read"]
      }
    ]
  },
  "kibana": [
    {
      "feature": {
        "siem": ["all"],
        "actions": ["read"], // change this to "none" to see the rule actions dropdown change
        "builtInAlerts": ["all"],
        "dev_tools": ["all"],
        "savedObjectsManagement": ["all"]
      },
      "spaces": ["*"]
    }
  ]
}'

This is what the "Rule Actions" step on the rule creation form looks like if the user does not have "read" privileges for actions:

And here it is on the edit page

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
• This was checked for keyboard-only and screenreader accessibility
• 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

[elasticmachine]

Pinging @elastic/siem (Team:SIEM)

[FrankHassanabad]

This all works as is. Just tested it, but instead of us showing the error message in the combo box and populating it and running the risk of us pushing the value from the combo box to the backend:

Can we change the code around here to remove the combo box and use regular text elements to display the error?

Somewhere down here add the conditional logic and html:

  return isReadOnlyView ? (
    <StepContentWrapper addPadding={addPadding}>
      <StepRuleDescription schema={schema} data={initialState} columns="single" />
    </StepContentWrapper>
  ) : (
    <>
      <StepContentWrapper addPadding={!isUpdateView}>
        <Form form={form} data-test-subj="stepRuleActions">
          <EuiForm>
            <UseField
              path="throttle"
              component={ThrottleSelectField}
              componentProps={throttleFieldComponentProps}
            />
            {throttle !== stepActionsDefaultValue.throttle ? (

[dhurley14]

Technically the value is still the default value of DEFAULT_THROTTLE_OPTION.value I'm just changing the text. I tried not rendering the form but then the submission button wanted a value for actions from that combo box So I figured rather than messing with the logic for submission and changing the request to the backend I would just update the text in the combo box and remove the options. I'll look into doing your suggestion though 👍

[FrankHassanabad]

If things begin to feel or get messy in here, feel ok to drop all the boolean logic weirdness with expressions. That is meant to be a helper for when things look very templatey and nice in React but as time marches on along with permission booleans and things begin getting complex where we have branching logic within branching logic and things look like a bunch of && and ! mashed together we can refactor out the expressions and do regular:

if/else if/else if/else

or if we want to we can do the regular:
if (condition) return this chunk of code early/if (second condition) return this chunk of code ...

Since that will allow us to execute more than one block of code within brackets compared to excessive and hard to reason about expressions within expressions and React doing odd things like adding {false/null/undefined<></>} to its tree to later parse out and remove.

I would avoid having elements with block vs none on them compared to just not displaying the component altogether based on conditional logic. Usually it is better to omit an HTML element altogether and DOM weight unless we have a bunch of DOM elements that are constantly being added and removed in which case doing a shown vs. being hidden is better (It will then prevent reflows from happening).

Already looking at this logic if it was me writing the code I would drop all the boolean template expressions and just use normal if/else if logic rather than mixing all these booleans together and trying to get them all correct.

Already we have these that cause us to keep a lot of state in our head:

• isReadOnlyView
• isUpdateView
• application.capabilities.actions.show

And the way the logic flows it's not clear branches that show relationships between the booleans but rather the booleans look rather disjointed from each other and nested within each other.

[FrankHassanabad]

Other tips is there is an unset css keyword if you're going this route you can utilize maybe instead of assuming that your form is always going to be a block:
https://developer.mozilla.org/en-US/docs/Web/CSS/unset#:~:text=The%20unset%20CSS%20keyword%20resets,its%20initial%20value%20if%20not.

Also you might be able to use destructoring to avoid assuming the block?

...{ application.capabilities.actions.show ? { display: 'block' } : {} }

But you will always render the form there when you have data as you introduce a new object but I don't think that's going to be a big deal.

[peluja1012]

I agree with @FrankHassanabad's comment here. We could just do something like this to avoid having to use style block or none, right?

{ application.capabilities.actions.show && (
   <Form ...>...</Form>
)}

[dhurley14]

The result of not rendering the actions step form is having to increase the complexity of the state of the whole rule creation form. The state of the rule creation form depends on pulling a default value from this and when the actions step form is not rendered, the rule creation form blows up. I'm going to look into using the GhostFormField and maybe rendering that with the throttle default somehow so that I don't need to mess with state.

Also a good thing to note, after talking with @rylnd I learned that we are submitting the entire form on edit. This implies that an analyst with no privileges on actions goes to edit a rule created by another analyst with actions privileges and they will be greeted with this error upon "save" in the "edit rules" page and the rule changes will not be saved.

[FrankHassanabad]

lgtm

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 782e0bd

Metrics [docs]

async chunks size

id	before	after	diff
securitySolution	10.3MB	10.3MB	+1.3KB

History

• 💔 Build #79202 failed ebbb0a7
• 💚 Build #78814 succeeded c24b0f4
• 💚 Build #78762 succeeded 87c194d
• 💚 Build #78438 succeeded 87c194d
• 💔 Build #78122 failed 26e12ac

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

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)