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][Detections][Tech Debt] - Move to using common io-ts types #75009

Merged
merged 12 commits into from
Sep 4, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ export const sortFieldOrUndefined = t.union([sort_field, t.undefined]);
export type SortFieldOrUndefined = t.TypeOf<typeof sortFieldOrUndefined>;

export const sort_order = t.keyof({ asc: null, desc: null });
export type sortOrder = t.TypeOf<typeof sort_order>;
export type SortOrder = t.TypeOf<typeof sort_order>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was just a typo? Seems like all other typescript ones are uppercase.


export const sortOrderOrUndefined = t.union([sort_order, t.undefined]);
export type SortOrderOrUndefined = t.TypeOf<typeof sortOrderOrUndefined>;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
export * from './add_prepackaged_rules_schema';
export * from './create_rules_bulk_schema';
export * from './create_rules_schema';
export * from './export_rules_schema';
export * from './find_rules_schema';
export * from './import_rules_schema';
export * from './patch_rules_bulk_schema';
export * from './patch_rules_schema';
export * from './query_rules_schema';
export * from './query_signals_index_schema';
export * from './set_signal_status_schema';
export * from './update_rules_bulk_schema';
export * from './update_rules_schema';
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
export * from './error_schema';
export * from './find_rules_schema';
export * from './import_rules_schema';
export * from './prepackaged_rules_schema';
export * from './prepackaged_rules_status_schema';
export * from './rules_bulk_schema';
export * from './rules_schema';
export * from './type_timeline_only_schema';
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
*/

import {
AddRulesProps,
PatchRuleProps,
NewRule,
CreateRulesProps,
UpdateRulesProps,
PrePackagedRulesStatusResponse,
BasicFetchProps,
RuleStatusResponse,
Expand All @@ -16,13 +16,18 @@ import {
FetchRulesResponse,
FetchRulesProps,
} from '../types';
import { ruleMock, savedRuleMock, rulesMock } from '../mock';
import { savedRuleMock, rulesMock } from '../mock';
import { getRulesSchemaMock } from '../../../../../../common/detection_engine/schemas/response/rules_schema.mocks';
import { RulesSchema } from '../../../../../../common/detection_engine/schemas/response';

export const addRule = async ({ rule, signal }: AddRulesProps): Promise<NewRule> =>
Promise.resolve(ruleMock);
export const updateRule = async ({ rule, signal }: UpdateRulesProps): Promise<RulesSchema> =>
Promise.resolve(getRulesSchemaMock());

export const patchRule = async ({ ruleProperties, signal }: PatchRuleProps): Promise<NewRule> =>
Promise.resolve(ruleMock);
export const createRule = async ({ rule, signal }: CreateRulesProps): Promise<RulesSchema> =>
Promise.resolve(getRulesSchemaMock());

export const patchRule = async ({ ruleProperties, signal }: PatchRuleProps): Promise<RulesSchema> =>
Promise.resolve(getRulesSchemaMock());

export const getPrePackagedRulesStatus = async ({
signal,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@

import { KibanaServices } from '../../../../common/lib/kibana';
import {
addRule,
createRule,
updateRule,
patchRule,
fetchRules,
fetchRuleById,
enableRules,
Expand All @@ -19,9 +21,12 @@ import {
fetchTags,
getPrePackagedRulesStatus,
} from './api';
import { ruleMock, rulesMock } from './mock';
import { getRulesSchemaMock } from '../../../../../common/detection_engine/schemas/response/rules_schema.mocks';
import { getCreateRulesSchemaMock } from '../../../../../common/detection_engine/schemas/request/create_rules_schema.mock';
import { getUpdateRulesSchemaMock } from '../../../../../common/detection_engine/schemas/request/update_rules_schema.mock';
import { getPatchRulesSchemaMock } from '../../../../../common/detection_engine/schemas/request/patch_rules_schema.mock';
import { rulesMock } from './mock';
import { buildEsQuery } from 'src/plugins/data/common';

const abortCtrl = new AbortController();
const mockKibanaServices = KibanaServices.get as jest.Mock;
jest.mock('../../../../common/lib/kibana');
Expand All @@ -30,25 +35,56 @@ const fetchMock = jest.fn();
mockKibanaServices.mockReturnValue({ http: { fetch: fetchMock } });

describe('Detections Rules API', () => {
describe('addRule', () => {
describe('createRule', () => {
beforeEach(() => {
fetchMock.mockClear();
fetchMock.mockResolvedValue(ruleMock);
fetchMock.mockResolvedValue(getRulesSchemaMock());
});

test('check parameter url, body', async () => {
await addRule({ rule: ruleMock, signal: abortCtrl.signal });
test('POSTs rule', async () => {
const payload = getCreateRulesSchemaMock();
await createRule({ rule: payload, signal: abortCtrl.signal });
expect(fetchMock).toHaveBeenCalledWith('/api/detection_engine/rules', {
body:
'{"description":"some desc","enabled":true,"false_positives":[],"filters":[],"from":"now-360s","index":["apm-*-transaction*","auditbeat-*","endgame-*","filebeat-*","packetbeat-*","winlogbeat-*"],"interval":"5m","rule_id":"bbd3106e-b4b5-4d7c-a1a2-47531d6a2baf","language":"kuery","risk_score":75,"name":"Test rule","query":"user.email: \'root@elastic.co\'","references":[],"severity":"high","tags":["APM"],"to":"now","type":"query","threat":[],"throttle":null}',
'{"description":"Detecting root and admin users","name":"Query with a rule id","query":"user.name: root or user.name: admin","severity":"high","type":"query","risk_score":55,"language":"kuery","rule_id":"rule-1"}',
method: 'POST',
signal: abortCtrl.signal,
});
});
});

test('happy path', async () => {
const ruleResp = await addRule({ rule: ruleMock, signal: abortCtrl.signal });
expect(ruleResp).toEqual(ruleMock);
describe('updateRule', () => {
beforeEach(() => {
fetchMock.mockClear();
fetchMock.mockResolvedValue(getRulesSchemaMock());
});

test('PUTs rule', async () => {
const payload = getUpdateRulesSchemaMock();
await updateRule({ rule: payload, signal: abortCtrl.signal });
expect(fetchMock).toHaveBeenCalledWith('/api/detection_engine/rules', {
body:
'{"description":"some description","name":"Query with a rule id","query":"user.name: root or user.name: admin","severity":"high","type":"query","risk_score":55,"language":"kuery","rule_id":"rule-1"}',
method: 'PUT',
signal: abortCtrl.signal,
});
});
});

describe('patchRule', () => {
beforeEach(() => {
fetchMock.mockClear();
fetchMock.mockResolvedValue(getRulesSchemaMock());
});

test('PATCHs rule', async () => {
const payload = getPatchRulesSchemaMock();
await patchRule({ ruleProperties: payload, signal: abortCtrl.signal });
expect(fetchMock).toHaveBeenCalledWith('/api/detection_engine/rules', {
body: JSON.stringify(payload),
method: 'PATCH',
signal: abortCtrl.signal,
});
});
});

Expand Down Expand Up @@ -280,7 +316,7 @@ describe('Detections Rules API', () => {
describe('fetchRuleById', () => {
beforeEach(() => {
fetchMock.mockClear();
fetchMock.mockResolvedValue(ruleMock);
fetchMock.mockResolvedValue(getRulesSchemaMock());
});

test('check parameter url, query', async () => {
Expand All @@ -296,7 +332,7 @@ describe('Detections Rules API', () => {

test('happy path', async () => {
const ruleResp = await fetchRuleById({ id: 'mySuperRuleId', signal: abortCtrl.signal });
expect(ruleResp).toEqual(ruleMock);
expect(ruleResp).toEqual(getRulesSchemaMock());
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { HttpStart } from '../../../../../../../../src/core/public';
import {
DETECTION_ENGINE_RULES_URL,
Expand All @@ -13,13 +12,13 @@ import {
DETECTION_ENGINE_TAGS_URL,
} from '../../../../../common/constants';
import {
AddRulesProps,
UpdateRulesProps,
CreateRulesProps,
DeleteRulesProps,
DuplicateRulesProps,
EnableRulesProps,
FetchRulesProps,
FetchRulesResponse,
NewRule,
Rule,
FetchRuleProps,
BasicFetchProps,
Expand All @@ -33,32 +32,51 @@ import {
} from './types';
import { KibanaServices } from '../../../../common/lib/kibana';
import * as i18n from '../../../pages/detection_engine/rules/translations';
import { RulesSchema } from '../../../../../common/detection_engine/schemas/response';

/**
* Add provided Rule
* Create provided Rule
*
* @param rule to add
* @param rule CreateRulesSchema to add
* @param signal to cancel request
*
* @throws An error if response is not OK
*/
export const addRule = async ({ rule, signal }: AddRulesProps): Promise<NewRule> =>
KibanaServices.get().http.fetch<NewRule>(DETECTION_ENGINE_RULES_URL, {
method: rule.id != null ? 'PUT' : 'POST',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither CreateRuleSchema or UpdateRuleSchema require the presence of an id so this check was no longer valid.

export const createRule = async ({ rule, signal }: CreateRulesProps): Promise<RulesSchema> =>
KibanaServices.get().http.fetch<RulesSchema>(DETECTION_ENGINE_RULES_URL, {
method: 'POST',
body: JSON.stringify(rule),
signal,
});

/**
* Update provided Rule using PUT
*
* @param rule UpdateRulesSchema to be updated
* @param signal to cancel request
*
* @throws An error if response is not OK
*/
export const updateRule = async ({ rule, signal }: UpdateRulesProps): Promise<RulesSchema> =>
KibanaServices.get().http.fetch<RulesSchema>(DETECTION_ENGINE_RULES_URL, {
method: 'PUT',
body: JSON.stringify(rule),
signal,
});

/**
* Patch provided Rule
* Patch provided rule
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to discourage usage of patch in a certain way, we should probably limit the schema so that we can only patch the fields that we're expecting to be patched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good call. I think it's particular to the way we built the UI (need to double check that) so maybe creating a limited schema in the front end makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly, usage of patch was highly discouraged in general... so maybe worth discussing limiting on the backend too? Shouldn't block this PR, but if we could prevent someone from trying to use the API in ways that could be detrimental, I think that could be worthwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, just circling back on this PR now. I think that's definitely a good point that we should discuss. I added it to our team discussion for next week.

* NOTE: The rule edit flow does NOT use patch as it relies on the
* functionality of PUT to delete field values when not provided, if
* just expecting changes, use this `patchRule`
*
* @param ruleProperties to patch
* @param signal to cancel request
*
* @throws An error if response is not OK
*/
export const patchRule = async ({ ruleProperties, signal }: PatchRuleProps): Promise<NewRule> =>
KibanaServices.get().http.fetch<NewRule>(DETECTION_ENGINE_RULES_URL, {
export const patchRule = async ({ ruleProperties, signal }: PatchRuleProps): Promise<RulesSchema> =>
KibanaServices.get().http.fetch<RulesSchema>(DETECTION_ENGINE_RULES_URL, {
method: 'PATCH',
body: JSON.stringify(ruleProperties),
signal,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

export * from './api';
export * from './fetch_index_patterns';
export * from './persist_rule';
export * from './use_update_rule';
export * from './use_create_rule';
export * from './types';
export * from './use_rule';
export * from './use_rules';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,36 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { NewRule, FetchRulesResponse, Rule } from './types';

export const ruleMock: NewRule = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving towards using the already created mocks next to the schema files.

description: 'some desc',
enabled: true,
false_positives: [],
filters: [],
from: 'now-360s',
index: [
'apm-*-transaction*',
'auditbeat-*',
'endgame-*',
'filebeat-*',
'packetbeat-*',
'winlogbeat-*',
],
interval: '5m',
rule_id: 'bbd3106e-b4b5-4d7c-a1a2-47531d6a2baf',
language: 'kuery',
risk_score: 75,
name: 'Test rule',
query: "user.email: 'root@elastic.co'",
references: [],
severity: 'high',
tags: ['APM'],
to: 'now',
type: 'query',
threat: [],
throttle: null,
};
import { FetchRulesResponse, Rule } from './types';

export const savedRuleMock: Rule = {
author: [],
Expand Down
Loading