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.

Sign up for GitHub

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] POC reusing types and avoiding data-driven #143383

Merged
merged 10 commits into from
Nov 17, 2022
Merged

[Security Solution] POC reusing types and avoiding data-driven #143383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fa6bdaf
POC reusing existing types + no data driven tests
MadameSheema Oct 14, 2022
8101d34
Merge branch 'main' into cypress/poc-schemas
kibanamachine Oct 18, 2022
5a48703
Merge branch 'main' into cypress/poc-schemas
MadameSheema Oct 20, 2022
1ecb770
accommodating schemas file before merging main to avoid conflicts
MadameSheema Nov 10, 2022
3af61c1
Merge branch 'main' into cypress/poc-schemas
MadameSheema Nov 10, 2022
604ce5e
updates types
MadameSheema Nov 10, 2022
db09f55
please comments
MadameSheema Nov 13, 2022
5e9a256
Merge branch 'main' into cypress/poc-schemas
kibanamachine Nov 16, 2022
5914b30
fixes
MadameSheema Nov 16, 2022
d8ab9e9
Merge branch 'cypress/poc-schemas' of github.com:MadameSheema/kibana …
MadameSheema Nov 16, 2022
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
88 changes: 88 additions & 0 deletions x-pack/plugins/security_solution/cypress/data/detection_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
banderror marked this conversation as resolved.
Show resolved Hide resolved
* 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 type {
RiskScore,
RuleInterval,
RuleIntervalFrom,
Severity,
Threat,
ThreatSubtechnique,
ThreatTechnique,
} from '@kbn/securitysolution-io-ts-alerting-types';

import type {
IndexPatternArray,
InvestigationGuide,
RuleDescription,
RuleFalsePositiveArray,
RuleQuery,
RuleName,
RuleReferenceArray,
RuleTagArray,
} from '../../common/detection_engine/rule_schema';

interface RuleFields {
defaultIndexPatterns: IndexPatternArray;
falsePositives: RuleFalsePositiveArray;
investigationGuide: InvestigationGuide;
referenceUrls: RuleReferenceArray;
riskScore: RiskScore;
ruleDescription: RuleDescription;
ruleInterval: RuleInterval;
ruleIntervalFrom: RuleIntervalFrom;
ruleQuery: RuleQuery;
ruleName: RuleName;
ruleTags: RuleTagArray;
ruleSeverity: Severity;
threat: Threat;
threatSubtechnique: ThreatSubtechnique;
threatTechnique: ThreatTechnique;
}

export const ruleFields: RuleFields = {
defaultIndexPatterns: [
'apm-*-transaction*',
'auditbeat-*',
'endgame-*',
'filebeat-*',
'logs-*',
'packetbeat-*',
'traces-apm*',
'winlogbeat-*',
'-*elastic-cloud-logs-*',
],
falsePositives: ['False1', 'False2'],
investigationGuide: '# test markdown',
referenceUrls: ['http://example.com/', 'https://example.com/'],
riskScore: 17,
ruleDescription: 'The rule description',
ruleInterval: '5m',
ruleIntervalFrom: '50000h',
ruleQuery: 'host.name: *',
ruleName: 'Test Rule',
ruleTags: ['test', 'newRule'],
ruleSeverity: 'high',
threat: {
framework: 'MITRE ATT&CK',
tactic: {
name: 'Credential Access',
id: 'TA0006',
reference: 'https://attack.mitre.org/tactics/TA0006',
},
},
threatSubtechnique: {
name: '/etc/passwd and /etc/shadow',
id: 'T1003.008',
reference: 'https://attack.mitre.org/techniques/T1003/008',
},
threatTechnique: {
id: 'T1003',
name: 'OS Credential Dumping',
reference: 'https://attack.mitre.org/techniques/T1003',
},
};
142 changes: 91 additions & 51 deletions x-pack/plugins/security_solution/cypress/e2e/detection_rules/custom_query_rule.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,15 @@
* 2.0.
*/

import { formatMitreAttackDescription } from '../../helpers/rules';
import type { Mitre } from '../../objects/rule';
import { ruleFields } from '../../data/detection_engine';
import {
getNewRule,
getExistingRule,
getIndexPatterns,
getEditedRule,
getNewOverrideRule,
} from '../../objects/rule';
import type { CompleteTimeline } from '../../objects/timeline';
import { getTimeline } from '../../objects/timeline';
import { ALERT_GRID_CELL, NUMBER_OF_ALERTS } from '../../screens/alerts';

import {
Expand Down Expand Up @@ -56,7 +55,6 @@ import {
INDEX_PATTERNS_DETAILS,
INVESTIGATION_NOTES_MARKDOWN,
INVESTIGATION_NOTES_TOGGLE,
MITRE_ATTACK_DETAILS,
REFERENCE_URLS_DETAILS,
RISK_SCORE_DETAILS,
RULE_NAME_HEADER,
Expand All @@ -66,6 +64,9 @@ import {
SEVERITY_DETAILS,
TAGS_DETAILS,
TIMELINE_TEMPLATE_DETAILS,
THREAT_TACTIC,
THREAT_TECHNIQUE,
THREAT_SUBTECHNIQUE,
} from '../../screens/rule_details';

import {
Expand All @@ -82,14 +83,26 @@ import { createTimeline } from '../../tasks/api_calls/timelines';
import { cleanKibana, deleteAlertsAndRules } from '../../tasks/common';
import { addEmailConnectorAndRuleAction } from '../../tasks/common/rule_actions';
import {
continueWithNextSection,
createAndEnableRule,
expandAdvancedSettings,
fillAboutRule,
fillAboutRuleAndContinue,
fillDefineCustomRuleAndContinue,
fillScheduleRuleAndContinue,
fillDescription,
fillFalsePositiveExamples,
fillFrom,
fillNote,
fillReferenceUrls,
fillRiskScore,
fillRuleName,
fillRuleTags,
fillSeverity,
fillThreat,
fillThreatSubtechnique,
fillThreatTechnique,
goToAboutStepTab,
goToActionsStepTab,
goToScheduleStepTab,
importSavedQuery,
waitForAlertsToPopulate,
waitForTheRuleToBeExecuted,
} from '../../tasks/create_new_rule';
Expand All @@ -105,98 +118,125 @@ describe('Custom query rules', () => {
login();
});
describe('Custom detection rules creation', () => {
const expectedUrls = getNewRule().referenceUrls?.join('');
const expectedFalsePositives = getNewRule().falsePositivesExamples?.join('');
const expectedTags = getNewRule().tags?.join('');
const mitreAttack = getNewRule().mitre as Mitre[];
const expectedMitre = formatMitreAttackDescription(mitreAttack);
const expectedNumberOfRules = 1;

beforeEach(() => {
const timeline = getNewRule().timeline as CompleteTimeline;
deleteAlertsAndRules();
createTimeline(timeline).then((response) => {
cy.wrap({
...getNewRule(),
timeline: {
...timeline,
id: response.body.data.persistTimeline.timeline.savedObjectId,
},
}).as('rule');
});
createTimeline(getTimeline())
.then((response) => {
return response.body.data.persistTimeline.timeline.savedObjectId;
})
.as('timelineId');
banderror marked this conversation as resolved.
Show resolved Hide resolved
});

it('Creates and enables a new rule', function () {
visit(RULE_CREATION);
fillDefineCustomRuleAndContinue(this.rule);
fillAboutRuleAndContinue(this.rule);
fillScheduleRuleAndContinue(this.rule);

cy.log('Filling define section');
importSavedQuery(this.timelineId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably an off-topic, because it seems like you kept the previous behavior of the test, but I think in a basic e2e test we should be filling in regular (non-saved) query.

Same for severity, risk score, and other defaultable or optional fields below.

I'd have one basic e2e test that fills in minimal (and only required) fields, maybe another one that fills in all the fields including optional and defaultable, and potentially a few special tests (like one for saved queries).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I kept the previous behavior, I listed it as a Next steps Agree if we need to fill ALL the fields for each one of the different creation rule tests. We can chat about it (although I believe we are on the same page). And do it in a follow-up PR to not compromise the current coverage we might have.

Copy link
Contributor

Choose a reason for hiding this comment

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

And do it in a follow-up PR to not compromise the current coverage we might have.

Sure, there's no rush with that 👍

continueWithNextSection();

cy.log('Filling about section');
fillRuleName();
fillDescription();
fillSeverity();
fillRiskScore();
fillRuleTags();
expandAdvancedSettings();
fillReferenceUrls();
fillFalsePositiveExamples();
fillThreat();
fillThreatTechnique();
fillThreatSubtechnique();
fillNote();
continueWithNextSection();
Comment on lines +140 to +152
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd group these into an object as well which would improve readability, encapsulate details and simplify imports:

aboutStep.fillRuleName();
aboutStep.fillDescription();
// etc


cy.log('Filling schedule section');
fillFrom();

// expect define step to repopulate
cy.get(DEFINE_EDIT_BUTTON).click();
cy.get(CUSTOM_QUERY_INPUT).should('have.value', this.rule.customQuery);
cy.get(CUSTOM_QUERY_INPUT).should('have.value', ruleFields.ruleQuery);
cy.get(DEFINE_CONTINUE_BUTTON).should('exist').click({ force: true });
cy.get(DEFINE_CONTINUE_BUTTON).should('not.exist');

// expect about step to populate
cy.get(ABOUT_EDIT_BUTTON).click();
cy.get(RULE_NAME_INPUT).invoke('val').should('eql', this.rule.name);
cy.get(RULE_NAME_INPUT).invoke('val').should('eql', ruleFields.ruleName);
cy.get(ABOUT_CONTINUE_BTN).should('exist').click({ force: true });
cy.get(ABOUT_CONTINUE_BTN).should('not.exist');
cy.get(SCHEDULE_CONTINUE_BUTTON).click({ force: true });

createAndEnableRule();

cy.log('Asserting we have a new rule created');
cy.get(CUSTOM_RULES_BTN).should('have.text', 'Custom rules (1)');

cy.log('Asserting rule view in rules list');
cy.get(RULES_TABLE).find(RULES_ROW).should('have.length', expectedNumberOfRules);
cy.get(RULE_NAME).should('have.text', this.rule.name);
cy.get(RISK_SCORE).should('have.text', this.rule.riskScore);
cy.get(SEVERITY).should('have.text', this.rule.severity);
cy.get(RULE_NAME).should('have.text', ruleFields.ruleName);
cy.get(RISK_SCORE).should('have.text', ruleFields.riskScore);
cy.get(SEVERITY)
.invoke('text')
.then((text) => {
cy.wrap(text.toLowerCase()).should('equal', ruleFields.ruleSeverity);
});
cy.get(RULE_SWITCH).should('have.attr', 'aria-checked', 'true');

goToRuleDetails();

cy.get(RULE_NAME_HEADER).should('contain', `${this.rule.name}`);
cy.get(ABOUT_RULE_DESCRIPTION).should('have.text', this.rule.description);
cy.log('Asserting rule details');
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry off-topic again, but I think this test does too much which is irrelevant to rule creation functionality.
In order to check if the rule was created properly it should be sufficient to

  1. fetch the created rule via the API and assert its fields
  2. validate that after creation the user is navigated to the rules page
  3. and the rules table is refreshed and shows the newly created rule

Rule Details page is a separate feature that should be covered by separate e2e tests.

cy.get(RULE_NAME_HEADER).should('contain', ruleFields.ruleName);
cy.get(ABOUT_RULE_DESCRIPTION).should('have.text', ruleFields.ruleDescription);
cy.get(ABOUT_DETAILS).within(() => {
getDetails(SEVERITY_DETAILS).should('have.text', this.rule.severity);
getDetails(RISK_SCORE_DETAILS).should('have.text', this.rule.riskScore);
getDetails(SEVERITY_DETAILS)
.invoke('text')
.then((text) => {
cy.wrap(text.toLowerCase()).should('equal', ruleFields.ruleSeverity);
});
getDetails(RISK_SCORE_DETAILS).should('have.text', ruleFields.riskScore);
getDetails(REFERENCE_URLS_DETAILS).should((details) => {
expect(removeExternalLinkText(details.text())).equal(expectedUrls);
});
getDetails(FALSE_POSITIVES_DETAILS).should('have.text', expectedFalsePositives);
getDetails(MITRE_ATTACK_DETAILS).should((mitre) => {
expect(removeExternalLinkText(mitre.text())).equal(expectedMitre);
expect(removeExternalLinkText(details.text())).equal(ruleFields.referenceUrls.join(''));
});
getDetails(TAGS_DETAILS).should('have.text', expectedTags);
getDetails(FALSE_POSITIVES_DETAILS).should('have.text', ruleFields.falsePositives.join(''));
getDetails(TAGS_DETAILS).should('have.text', ruleFields.ruleTags.join(''));
});
cy.get(THREAT_TACTIC).should(
'contain',
`${ruleFields.threat.tactic.name} (${ruleFields.threat.tactic.id})`
);
cy.get(THREAT_TECHNIQUE).should(
'contain',
`${ruleFields.threatTechnique.name} (${ruleFields.threatTechnique.id})`
);
cy.get(THREAT_SUBTECHNIQUE).should(
'contain',
`${ruleFields.threatSubtechnique.name} (${ruleFields.threatSubtechnique.id})`
);
cy.get(INVESTIGATION_NOTES_TOGGLE).click({ force: true });
cy.get(ABOUT_INVESTIGATION_NOTES).should('have.text', INVESTIGATION_NOTES_MARKDOWN);
cy.get(DEFINITION_DETAILS).within(() => {
getDetails(INDEX_PATTERNS_DETAILS).should('have.text', getIndexPatterns().join(''));
getDetails(CUSTOM_QUERY_DETAILS).should('have.text', this.rule.customQuery);
getDetails(INDEX_PATTERNS_DETAILS).should(
'have.text',
ruleFields.defaultIndexPatterns.join('')
);
getDetails(CUSTOM_QUERY_DETAILS).should('have.text', ruleFields.ruleQuery);
getDetails(RULE_TYPE_DETAILS).should('have.text', 'Query');
getDetails(TIMELINE_TEMPLATE_DETAILS).should('have.text', 'None');
});
cy.get(SCHEDULE_DETAILS).within(() => {
getDetails(RUNS_EVERY_DETAILS).should(
'have.text',
`${getNewRule().runsEvery?.interval}${getNewRule().runsEvery?.type}`
);
getDetails(ADDITIONAL_LOOK_BACK_DETAILS).should(
'have.text',
`${getNewRule().lookBack?.interval}${getNewRule().lookBack?.type}`
);
getDetails(RUNS_EVERY_DETAILS).should('have.text', ruleFields.ruleInterval);
getDetails(ADDITIONAL_LOOK_BACK_DETAILS).should('have.text', ruleFields.ruleIntervalFrom);
});

waitForTheRuleToBeExecuted();
waitForAlertsToPopulate();

cy.log('Asserting that alerts have been generated after the creation');
cy.get(NUMBER_OF_ALERTS)
.invoke('text')
.should('match', /^[1-9].+$/); // Any number of alerts
cy.get(ALERT_GRID_CELL).contains(this.rule.name);
cy.get(ALERT_GRID_CELL).contains(ruleFields.ruleName);
});
});

Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ export const DATA_VIEW_COMBO_BOX =

export const DATA_VIEW_OPTION = '[data-test-subj="rule-index-toggle-dataView"]';

export const CONTINUE_BUTTON = '[data-test-subj$=-continue]';

export const DEFINE_CONTINUE_BUTTON = '[data-test-subj="define-continue"]';

export const DEFINE_EDIT_BUTTON = '[data-test-subj="edit-define-rule"]';
Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugins/security_solution/cypress/screens/rule_details.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,9 @@ export const DEFINE_RULE_PANEL_PROGRESS =
'[data-test-subj="defineRule"] [data-test-subj="stepPanelProgress"]';

export const EDIT_RULE_SETTINGS_LINK = '[data-test-subj="editRuleSettingsLink"]';

export const THREAT_TACTIC = '[data-test-subj="threatTacticLink"]';

export const THREAT_TECHNIQUE = '[data-test-subj="threatTechniqueLink"]';

export const THREAT_SUBTECHNIQUE = '[data-test-subj="threatSubtechniqueLink"]';
Loading