Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] Extend Prebuilt rules install and update workflow test coverage #161687

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .buildkite/ftr_configs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ enabled:
- x-pack/test/detection_engine_api_integration/security_and_spaces/prebuilt_rules/config.ts
- x-pack/test/detection_engine_api_integration/security_and_spaces/bundled_prebuilt_rules_package/config.ts
- x-pack/test/detection_engine_api_integration/security_and_spaces/large_prebuilt_rules_package/config.ts
- x-pack/test/detection_engine_api_integration/security_and_spaces/update_prebuilt_rules_package/config.ts
- x-pack/test/encrypted_saved_objects_api_integration/config.ts
- x-pack/test/examples/config.ts
- x-pack/test/fleet_api_integration/config.agent.ts
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to also check if a user with a required minimum of permissions can install and update rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in #165488

Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { APP_PATH, RULES_ADD_PATH, RULES_UPDATES } from '../../../common/constants';
import { createRuleAssetSavedObject } from '../../helpers/rules';
import { waitForRulesTableToBeLoaded } from '../../tasks/alerts_detection_rules';
import { createAndInstallMockedPrebuiltRules } from '../../tasks/api_calls/prebuilt_rules';
import { resetRulesTableState, deleteAlertsAndRules } from '../../tasks/common';
import { esArchiverResetKibana } from '../../tasks/es_archiver';
import { login, waitForPageWithoutDateRange } from '../../tasks/login';
import { SECURITY_DETECTIONS_RULES_URL } from '../../urls/navigation';
import { ROLES } from '../../../common/test';
import {
ADD_ELASTIC_RULES_BTN,
getInstallSingleRuleButtonByRuleId,
getUpgradeSingleRuleButtonByRuleId,
INSTALL_ALL_RULES_BUTTON,
RULES_UPDATES_TAB,
RULE_CHECKBOX,
UPGRADE_ALL_RULES_BUTTON,
} from '../../screens/alerts_detection_rules';

const RULE_1_ID = 'rule_1';
const RULE_2_ID = 'rule_2';
const OUTDATED_RULE_1 = createRuleAssetSavedObject({
name: 'Outdated rule 1',
rule_id: RULE_1_ID,
version: 1,
});
const UPDATED_RULE_1 = createRuleAssetSavedObject({
name: 'Updated rule 1',
rule_id: RULE_1_ID,
version: 2,
});
const OUTDATED_RULE_2 = createRuleAssetSavedObject({
name: 'Outdated rule 2',
rule_id: RULE_2_ID,
version: 1,
});
const UPDATED_RULE_2 = createRuleAssetSavedObject({
name: 'Updated rule 2',
rule_id: RULE_2_ID,
version: 2,
});
Comment on lines +29 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should only one outdated rule suffice for these tests?


const loadPageAsReadOnlyUser = (url: string) => {
login(ROLES.reader);
waitForPageWithoutDateRange(url, ROLES.reader);
};

describe('Detection rules, Prebuilt Rules Installation and Update - Authorization/RBAC', () => {
beforeEach(() => {
login();
resetRulesTableState();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need resetRulesTableState here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't, removing in #165488

deleteAlertsAndRules();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we delete all existing rules and create new ones only once in before instead of doing that multiple times using beforeEach?

esArchiverResetKibana();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need esArchiverResetKibana here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't, removing in #165488

waitForRulesTableToBeLoaded();
createAndInstallMockedPrebuiltRules({ rules: [OUTDATED_RULE_1, OUTDATED_RULE_2] });
});

describe('User with read privileges on Security Solution', () => {
const RULE_1 = createRuleAssetSavedObject({
name: 'Test rule 1',
rule_id: 'rule_1',
});
const RULE_2 = createRuleAssetSavedObject({
name: 'Test rule 2',
rule_id: 'rule_2',
});
beforeEach(() => {
// Now login with read-only user in preparation for test
createAndInstallMockedPrebuiltRules({ rules: [RULE_1, RULE_2], installToKibana: false });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understanding the final state of the rules table can be a bit challenging. This is because some rules are set up in the top-level beforeEach, while others are installed in the beforeEach at this level. To grasp the final state, you have to flip back and forth between the two sections and the body of the test. Moreover, it is not clear why outdated rules are required for this particular test case (rule installation). Could we do the precondition setup in one location for clarity?

For this entire suite, we need 1 outdated rule and 1 ready for installation. Let's do this setup once for both test cases in the before section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all the setup for this is being simplified in #165488

loadPageAsReadOnlyUser(SECURITY_DETECTIONS_RULES_URL);
waitForRulesTableToBeLoaded();
});

it('should not be able to install prebuilt rules', () => {
// Check that Add Elastic Rules button is disabled
cy.get(ADD_ELASTIC_RULES_BTN).should('be.disabled');

// Navigate to Add Elastic Rules page anyways via URL
// and assert that rules cannot be selected and all
// installation buttons are disabled
cy.visit(`${APP_PATH}${RULES_ADD_PATH}`);
cy.get(INSTALL_ALL_RULES_BUTTON).should('be.disabled');
cy.get(getInstallSingleRuleButtonByRuleId(RULE_1['security-rule'].rule_id)).should(
'not.exist'
);
cy.get(RULE_CHECKBOX).should('not.exist');
});
});

describe('User with read privileges on Security Solution', () => {
beforeEach(() => {
/* Create a second version of the rule, making it available for update */
createAndInstallMockedPrebuiltRules({
rules: [UPDATED_RULE_1, UPDATED_RULE_2],
installToKibana: false,
});
// Now login with read-only user in preparation for test
loadPageAsReadOnlyUser(SECURITY_DETECTIONS_RULES_URL);
waitForRulesTableToBeLoaded();
});

it('should not be able to upgrade prebuilt rules', () => {
// Check that Rule Update tab is not shown
cy.get(RULES_UPDATES_TAB).should('not.exist');

// Navigate to Rule Update tab anyways via URL
// and assert that rules cannot be selected and all
// upgrade buttons are disabled
cy.visit(`${APP_PATH}${RULES_UPDATES}`);
cy.get(UPGRADE_ALL_RULES_BUTTON).should('be.disabled');
cy.get(getUpgradeSingleRuleButtonByRuleId(OUTDATED_RULE_1['security-rule'].rule_id)).should(
'not.exist'
);
cy.get(RULE_CHECKBOX).should('not.exist');
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { createRuleAssetSavedObject } from '../../helpers/rules';
import { waitForRulesTableToBeLoaded } from '../../tasks/alerts_detection_rules';
import { createAndInstallMockedPrebuiltRules } from '../../tasks/api_calls/prebuilt_rules';
import { resetRulesTableState, deleteAlertsAndRules, reload } from '../../tasks/common';
import { esArchiverResetKibana } from '../../tasks/es_archiver';
import { login, visitWithoutDateRange } from '../../tasks/login';
import { SECURITY_DETECTIONS_RULES_URL } from '../../urls/navigation';
import {
addElasticRulesButtonClick,
assertRuleAvailableForInstallAndInstallOne,
assertRuleAvailableForInstallAndInstallSelected,
assertRuleAvailableForInstallAndInstallAllInPage,
assertRuleAvailableForInstallAndInstallAll,
assertRuleUpgradeAvailableAndUpgradeOne,
assertRuleUpgradeAvailableAndUpgradeSelected,
assertRuleUpgradeAvailableAndUpgradeAllInPage,
assertRuleUpgradeAvailableAndUpgradeAll,
ruleUpdatesTabClick,
} from '../../tasks/prebuilt_rules';

describe('Detection rules, Prebuilt Rules Installation and Update - Error handling', () => {
beforeEach(() => {
login();
resetRulesTableState();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need resetRulesTableState?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't, removing in #165488

deleteAlertsAndRules();
esArchiverResetKibana();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need esArchiverResetKibana?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't, removing in #165488


visitWithoutDateRange(SECURITY_DETECTIONS_RULES_URL);
});

describe('Installation of prebuilt rules - Should fail gracefully with toast error message when', () => {
const RULE_1 = createRuleAssetSavedObject({
name: 'Test rule 1',
rule_id: 'rule_1',
});
const RULE_2 = createRuleAssetSavedObject({
name: 'Test rule 2',
rule_id: 'rule_2',
});
beforeEach(() => {
createAndInstallMockedPrebuiltRules({ rules: [RULE_1, RULE_2], installToKibana: false });
waitForRulesTableToBeLoaded();
});

it('installing prebuilt rules one by one', () => {
addElasticRulesButtonClick();
assertRuleAvailableForInstallAndInstallOne({ rules: [RULE_1], didRequestFail: true });
});

it('installing multiple selected prebuilt rules by selecting them individually', () => {
addElasticRulesButtonClick();
assertRuleAvailableForInstallAndInstallSelected({
rules: [RULE_1, RULE_2],
didRequestFail: true,
});
});

it('installing multiple selected prebuilt rules by selecting all in page', () => {
addElasticRulesButtonClick();
assertRuleAvailableForInstallAndInstallAllInPage({
rules: [RULE_1, RULE_2],
didRequestFail: true,
});
});

it('installing all available rules at once', () => {
addElasticRulesButtonClick();
assertRuleAvailableForInstallAndInstallAll({ rules: [RULE_1, RULE_2], didRequestFail: true });
});
Comment on lines +52 to +76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current structure of these test cases makes it challenging to understand what's being tested. Functions such as assertRuleAvailableForInstallAndInstallSelected contain multiple user actions and branched logic. I find it hard to visualize the user path that's being tested in a simple, comprehensible manner, like:

  • Do A
  • Do B
  • Do C
  • Check outcome

Could we reflect this more directly in the code? For an error-testing scenario, I would expect something like:

it('installing multiple selected prebuilt rules by selecting all in page', () => {
  installMockRules([RULE_1, RULE_2]);
  goToRulesInstallPage();
  selectAllRulesInPage();
  clickInstallSelectedRulesButton();
  failInstallRulesRequest();
  assertErrorToastIsShown(rulesCount);
});

This way, the code narrates a clear story of user actions and the expected outcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplifing in #165488

});

describe('Update of prebuilt rules - Should fail gracefully with toast error message when', () => {
const RULE_1_ID = 'rule_1';
const RULE_2_ID = 'rule_2';
const OUTDATED_RULE_1 = createRuleAssetSavedObject({
name: 'Outdated rule 1',
rule_id: RULE_1_ID,
version: 1,
});
const UPDATED_RULE_1 = createRuleAssetSavedObject({
name: 'Updated rule 1',
rule_id: RULE_1_ID,
version: 2,
});
const OUTDATED_RULE_2 = createRuleAssetSavedObject({
name: 'Outdated rule 2',
rule_id: RULE_2_ID,
version: 1,
});
const UPDATED_RULE_2 = createRuleAssetSavedObject({
name: 'Updated rule 2',
rule_id: RULE_2_ID,
version: 2,
});
beforeEach(() => {
/* Create a new rule and install it */
createAndInstallMockedPrebuiltRules({ rules: [OUTDATED_RULE_1, OUTDATED_RULE_2] });
/* Create a second version of the rule, making it available for update */
createAndInstallMockedPrebuiltRules({
rules: [UPDATED_RULE_1, UPDATED_RULE_2],
installToKibana: false,
});
waitForRulesTableToBeLoaded();
reload();
});

it('upgrading prebuilt rules one by one', () => {
ruleUpdatesTabClick();
assertRuleUpgradeAvailableAndUpgradeOne({ rules: [OUTDATED_RULE_1], didRequestFail: true });
});

it('upgrading multiple selected prebuilt rules by selecting them individually', () => {
ruleUpdatesTabClick();
assertRuleUpgradeAvailableAndUpgradeSelected({
rules: [OUTDATED_RULE_1, OUTDATED_RULE_2],
didRequestFail: true,
});
});

it('upgrading multiple selected prebuilt rules by selecting all in page', () => {
ruleUpdatesTabClick();
assertRuleUpgradeAvailableAndUpgradeAllInPage({
rules: [OUTDATED_RULE_1, OUTDATED_RULE_2],
didRequestFail: true,
});
});

it('upgrading all rules with available upgrades at once', () => {
ruleUpdatesTabClick();
assertRuleUpgradeAvailableAndUpgradeAll({
rules: [OUTDATED_RULE_1, OUTDATED_RULE_2],
didRequestFail: true,
});
});
});
});
Loading