[ResponseOps][Rules] Prevent internally managed rule types to be disabled/enabled from the APIs #236672

cnasikas · 2025-09-27T10:07:40Z

Summary

This PR prevents internally managed rule types from being enabled/disabled through the Rule Disable, Enable and Bulk Enable and Disable APIs. The prevention logic is on he route level because we want to support updating internally managed rules from the alerts client.

Partial fixes: #222101

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2025-09-27T13:15:31Z

Pinging @elastic/response-ops (Team:ResponseOps)

cnasikas · 2025-09-28T08:30:20Z

@elasticmachine merge upstream

elasticmachine · 2025-09-28T10:22:45Z

💔 Build Failed

Buildkite Build
Commit: ab685d8

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #42 / alerting api integration security and spaces enabled Alerts - Group 5 bulk untrack should create new alerts if run rules again after alerts are untracked
[job] [logs] FTR Configs #80 / Fleet Endpoints Integrations inputs_with_standalone_docker_agent generate a valid config for standalone agents
[job] [logs] FTR Configs #80 / Fleet Endpoints Integrations inputs_with_standalone_docker_agent generate a valid config for standalone agents

Metrics [docs]

✅ unchanged

History

💔 Build #344610 failed 8eb4519

cc @cnasikas

adcoelho · 2025-09-29T07:08:18Z

...form/plugins/shared/alerting/server/routes/lib/validate_internal_rule_types_by_query.test.ts

+import { rulesClientMock } from '../../mocks';
+import { validateInternalRuleTypesByQuery } from './validate_internal_rule_types_by_query';
+
+describe('validateInternalRuleTypesByQuery', () => {


nit: missing happy path test.

adcoelho · 2025-09-29T07:18:38Z

.../plugins/shared/alerting/server/routes/rule/apis/bulk_enable/bulk_enable_rules_route.test.ts

  });
+
+  describe('internally managed rule types', () => {
+    it('throws 400 if the rule type is internally managed', async () => {


Reading these for the different APIs, I noticed you don't test the "Rule type not found" error path. I think it is fine, but I just wanna confirm. For each of these routes, the rulesClient does that check too, right?

adcoelho · 2025-09-29T07:25:20Z

.../platform/plugins/shared/alerting/server/routes/lib/validate_internal_rule_types_by_query.ts

+import type { RegistryRuleType } from '../../rule_type_registry';
+import type { RulesClient } from '../../rules_client';
+
+export const validateInternalRuleTypesByQuery = async ({


I feel like we had this conversation, but I don't remember the outcome 😅

I agree that the check should be done at the route level, but why is this function not defined in the rules client itself?

The routes would then do

rulesClient.validateInternalRuleTypesByQuery({ req: body, ruleTypes, operationText: 'disable', }) const bulkDisableResults = await rulesClient.bulkDisableRules({ filter, ids, untrack });

Or something like that.

adcoelho · 2025-09-29T07:29:09Z

x-pack/platform/plugins/shared/alerting/server/routes/rule/apis/disable/disable_rule_route.ts

        const disableParams = { id, untrack };

        try {
+          const rule = await rulesClient.get({ id });


Why don't these(disable+enable) use validateInternalRuleTypesByQuery? Is it because of getRuleTypesByQuery? The performance should be fine if filter is optional and only an id is passed.

The validateInternalRuleTypesByQuery does an ES aggregation to see if the rule is internal. I find it simpler to reason, explicit, and more performant if we do a single check. Also, if in the future the validateInternalRuleTypesByQuery changes to a way that supports only bulk edits, we will not have to go to all single routes and change the logic.

adcoelho · 2025-09-29T07:31:14Z

...k/platform/test/alerting_api_integration/security_and_spaces/group2/tests/alerting/update.ts

-
      it('should throw 400 error when trying to update an internally managed rule type', async () => {
-        const { body: createdRule1 } = await supertest
+        const { body: createdRule } = await supertest


This test is for update but the change was done to bulk_edit_rules_route. Should a test also be added to x-pack/platform/test/alerting_api_integration/security_and_spaces/group3/tests/alerting/bulk_edit.ts?

The tests for the bulk_edit and update were introduced in this #234937. Here, I fixed a bug in the test, I realized.

adcoelho · 2025-09-29T07:34:35Z

.../platform/plugins/shared/alerting/server/routes/lib/validate_internal_rule_types_by_query.ts

+  const ruleTypesByQuery = await rulesClient.getRuleTypesByQuery({ filter, ids });
+  const ruleTypeIds = new Set(ruleTypesByQuery.ruleTypes);
+
+  for (const ruleTypeId of ruleTypeIds) {


If this validation is for bulk operations, maybe we should add the errors to an array and only throw at the end with all the failing rules? wdyt?

I say "maybe" because if the user sets the filter option, it isn't very helpful.

However, in a scenario where multiple IDs are passed, if we throw the first one, the user removes it from the list, and then the API throws again, and again. It is not great UX.

Yes, it is not possible with the filter option to do it, but I would give it a try for the ids.

jcger · 2025-09-29T07:31:04Z

.../platform/plugins/shared/alerting/server/routes/lib/validate_internal_rule_types_by_query.ts

+    const ruleType = ruleTypes.get(ruleTypeId);
+
+    if (!ruleType) {
+      throw Boom.badRequest('Rule type not found');


you could include the rule type id here, should help to debug if needed

jcger · 2025-09-29T07:36:39Z

.../platform/plugins/shared/alerting/server/routes/lib/validate_internal_rule_types_by_query.ts

+    }
+
+    if (ruleType.internallyManaged) {
+      throw Boom.badRequest(


I have no time to test it, so I might be wrong here. But if we call a bulk action, let's say for all rules. What exactly happens here? For me it looks like, if there is an internal rule type, it will throw, not being able to run that bulk action on the other rules (those belonging to non internally managed types). This would make the whole bulk action useless as soon as there is an internally managed rule type

This is correct, and it is the intended behavior. It is much better to fail early for all than to introduce side effects where some of them are updated and some are not. Aside from the complexity of handling the various cases, the route should be idempotent on every call and failing early, regardless of achieving that.

Couldn't we ignore internally managed rules by filtering them out? That should keep it idempotent. We don't have to throw an error once we get to internally managed rule type. It's also worsening UX: users with internally managed rules won't be able to bulk edit

An API should be clear on its behavior. Filtering out internal rule types would confuse users because it is not clear why they are not updated (the API returned 200). I get the UX issue, which I agree with, but I think this should be addressed on the UI level. Btw, all prior validations work like this. If one of the rules fails to pass a condition, the whole API throws.

I will open an issue for the UX/UI. We need to make it clear that the rule is internally managed.

adcoelho

Tested and works as expected 👍

cnasikas added 2 commits September 27, 2025 13:05

Prevent disabling internal rule types

edfe137

Prevent enabling internal rule types

44046bb

cnasikas self-assigned this Sep 27, 2025

cnasikas added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v9.2.0 labels Sep 27, 2025

cnasikas added 7 commits September 27, 2025 13:34

Move validateInternalRuleTypes to lib

52be59f

Prevent bulk disabling internal rule types

59ad590

Prevent bulk enabling internal rule types

eefafa3

Fix update test

6fa92e1

Add integration test for enable

b371721

Add integration test for disable

bbf0bb3

Add bulk enable/disable integration tests

8eb4519

cnasikas marked this pull request as ready for review September 27, 2025 13:15

cnasikas requested a review from a team as a code owner September 27, 2025 13:15

elasticmachine and others added 2 commits September 28, 2025 10:30

Merge branch 'main' into internal_bulk_enable_disable

0848410

Rename util function

ab685d8

cnasikas mentioned this pull request Sep 28, 2025

Prevent almost all significant event rule mutations #222101

Open

2 tasks

adcoelho reviewed Sep 29, 2025

View reviewed changes

jcger reviewed Sep 29, 2025

View reviewed changes

adcoelho approved these changes Sep 29, 2025

View reviewed changes

[ResponseOps][Rules] Prevent internally managed rule types to be disabled/enabled from the APIs #236672

Are you sure you want to change the base?

[ResponseOps][Rules] Prevent internally managed rule types to be disabled/enabled from the APIs #236672

Conversation

cnasikas commented Sep 27, 2025

Summary

Checklist

Uh oh!

elasticmachine commented Sep 27, 2025

Uh oh!

cnasikas commented Sep 28, 2025

Uh oh!

elasticmachine commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

History

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcger Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adcoelho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticmachine commented Sep 28, 2025 •

edited

Loading

jcger Sep 29, 2025 •

edited

Loading

cnasikas Sep 29, 2025 •

edited

Loading