[Security Solution] [Detections] Disable edit button when user does not have actions privileges w/ rule + actions

[dhurley14]

Summary

When a user with no actions privileges visits the rule details page of a rule with actions, the following capabilities will be disabled on the UI:

Edit Rule Settings button

"Edit Rule Settings" button will be greyed out similarly to when the a user does not have CRUD privileges for SIEM.

and on the all rules table:

Activate switch

Disabling the activate switch on rules with actions on all rules table

Disabling the activate switch on rules with actions on rule details page

Duplicate rule

Disable duplicate ability on all rules table

Disable duplicate ability on rule details page

Bulk actions

Disable the bulk functionality of duplicate, activate, deactivate

Checklist

Delete any items that are not applicable to this PR.

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

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

[elasticmachine]

Pinging @elastic/siem (Team:SIEM)

[spong]

We'll have to account for a the Edit rule settings link from the All Rules table as well. Since this requires knowledge of whether or not a Rule has actions, if we can't fetch that as part of the overall rule data, we may want to just lock down the edit rules page similar as to what we do with immutable rules (tabs disabled), and put a banner at the top explaining why. This also covers the case for manually entering the URL (or clicking a saved link).

Side question -- did there end up being any API-layer checks preventing users from modifying a rule with actions if they don't have actions privileges?

[dhurley14]

Okay I'll make those updates for the all rules table too. As for api-side, the user would see a 403 error on the api if they attempt to edit a rule with an action and don't have privileges. So no, I have not made modifications on the api side to prevent users from editing. I can though if that would make for a better user experience.

[spong]

Looks like we'll need to disable the duplicate action as well since it hits the actions API. We could allow duplication sans actions, but to keep this fix simple for 7.10 let's just disable the duplicate action and we can re-visit finer grained permissions as needed.

Rule Details / All Rules

[spong]

nit: Would be nice to disable the Save changes button since it will result in an error, but not a big deal since the user needs to paste in the URL to get to this page in the first place.

[spong]

When we chatted earlier I thought we determined activate/deactivate would function without actions permissions, but I'm seeing it error out attempting to deactivate -- is this correct?

[dhurley14]

@spong Yeah I am also seeing this behavior. I was thinking that since the enable API route for the alerting framework was a separate call from updating that this would not have been a problem. I tested this situation outside of our code using the alerting _enable API and got the same response.

$ curl -s -k   -H 'Content-Type: application/json'   -H 'kbn-xsrf: 123'   -u ${ELASTICSEARCH_USERNAME}:${ELASTICSEARCH_PASSWORD}   -X POST ${KIBANA_URL}${SPACE_URL}/api/alerts/alert/fe195d20-0a21-4c76-87ea-e2ae03e1355a/_enable   | jq -S .;
{
  "error": "Forbidden",
  "message": "Unauthorized to execute actions",
  "statusCode": 403
}

This is my oversight. I actually had not tested this before we spoke but I assumed it would work since they were separate api's. After looking at the code it makes sense why we would get a 403 back from alerting and why alerting would prevent the enabling of an alert + action for a user that does not have any privileges for actions, otherwise the action would silently fail to execute since that new user's api key would be used to execute the alert + action. I'm assuming I should disable the enable switch for rules and use the same tooltip as the "Edit Rule" button?

[spong]

Thanks for all the fixes here @dhurley14! 🙂

In testing the latest I found one last touch point for activate/deactivate and that's with the bulk actions:

We've already got a few permissions checks around the bulk actions, so should be fairly straightforward to add something like:

  const containsActions = selectedRuleIds.some(
    (id) => rules.find((r) => r.id === id)?.actions?.length > 0 ?? false
  );

around here:

kibana/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/batch_actions.tsx

Lines 43 to 45 in 7f3ec3f

	const containsEnabled = selectedRuleIds.some(
	(id) => rules.find((r) => r.id === id)?.enabled ?? false
	);

And then disable the Activate/Deactivate/Duplicate actions if containsActions === true

[spong]

nit: Could simplify with if you want:

Suggested change

	if (rule.actions != null && rule.actions.length > 0 && isBoolean(privileges)) {
	if (rule.actions?.length > 0 > 0 && isBoolean(privileges)) {

[spong]

nit: Could move EuiToolTip to wrap this instead of EuiContextMenuItem in the event we end up adding a tooltip to the next menu item as well (to prevent the styling issue). This also ensure's all top level components have a key as well.

[spong]

This is a bit difficult to follow -- to determine the value of canBulkActivateRulesWithActions, we really just need to know if the user hasActionsPrivileges, and if any of the selected rules have actions, right?

With this logic we're going to loop over every rule even if the first rule has actions and the user doesn't have permissions, even though that's enough to know that we need to disable any bulk_action that requires action privileges.

Still making use of your canEditRuleWithActions() function, we can invert the check using some() such that we'll bail early as soon as we find one rule with actions and the user doesn't hasActionsPrivileges.

  const missingActionPrivileges = selectedRuleIds.some((id) => {
    return !canEditRuleWithActions(
      rules.find((rule) => rule.id === id),
      hasActionsPrivileges
    );
  });

Going further, if the user hasActionsPrivileges we don't even have to check if any rule has actions, so we can short circuit even earlier! At the expense of readability of course... 😅

  const missingActionPrivileges =
    !hasActionsPrivileges &&
    selectedRuleIds.some((id) => {
      return !canEditRuleWithActions(
        rules.find((rule) => rule.id === id),
        hasActionsPrivileges
      );
    });

And then you can replace !canBulkActivateRulesWithActions with missingActionPrivileges, which reads a bit easier too since it avoids negation (although it adds one back into the calculation, but that can't be avoided since we're inverting our check).

[spong]

Left a few nits for readability (which is tough with all these permissions checks), but overall LGTM! 👍

Thanks for taking care of all these corner cases too @dhurley14 -- this is going to be really helpful for our users! And as we chatted about (#80601), outside of this bugfix we can probably clean up a lot of this readability logic by having a custom actions component that takes a common set of privileges and determines its disabled state from that. That'll be a good refactor for a future release 🙂

[kibanamachine]

💚 Build Succeeded

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

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
securitySolution	2057	2058	+1

async chunks size

id	before	after	diff
securitySolution	8.1MB	8.1MB	+6.1KB

History

• 💚 Build #82459 succeeded c89f412
• 💚 Build #82429 succeeded 8c769dd85f594d8c2427db9ac6d8a6020c731be4
• 💔 Build #82169 failed bc015bebb7436b06450f351bea95728dc9c4c0db
• 💔 Build #82025 failed 9367003b1478c716813243f46b14472ac2957312
• 💔 Build #81978 failed f1733c473c583a9fe070287ab3070072889ffb0a

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

[elasticmachine]

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