[Security Solution][Detections][Tech Debt] - Move to using common io-ts types #75009

yctercero · 2020-08-13T22:27:29Z

Summary

Part of the DE tech debt includes moving to use the common io-ts types in the UI. Currently, we are using some of them in some places and other times we're using the front end defined typescript types. This means that changes often have to be done on both ends, validation isn't being done at the UI boundary on request or response. Using the common types could help us avoid bugs moving forward. Obviously, there's a lot of code to touch so trying to just do it piece by piece. This PR addresses the following:

Replaced uses of NewRule with common type CreateRuleSchema and UpdateRuleSchema. These were being combined a bit
Split usePersistRule which was used for both POST and PUT into two hooks - useCreateRule and useUpdateRule.
- The logic for combining these two relied on checking for the existence of an id - if present it would PUT, otherwise it would POST.
- However, id is not a required property for either of these, so it's not reliable
Updated the rule edit flow to use useUpdateRule
Updated the rule creation flow to use useCreateRule
~~Added request and response validation for rule PUT, POST, and PATCH~~ Edit: decided not to do this to move this PR forward, may need more discussion as to the pros/cons of added io-ts validation in the front

Testing

test creating a rule with just required fields
test creating a rule with all fields filled
test updating a rule
test adding an exception to an item for the first time (it uses patch to associate the exception list with the rule)

Checklist

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

yctercero · 2020-08-13T22:48:10Z

x-pack/plugins/security_solution/public/detections/containers/detection_engine/rules/api.ts

 * @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',


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

yctercero · 2020-08-14T00:03:23Z

x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts

@@ -325,7 +325,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>;


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

elasticmachine · 2020-08-14T00:19:05Z

Pinging @elastic/siem (Team:SIEM)

madirey · 2020-08-14T20:08:53Z

x-pack/plugins/security_solution/public/detections/containers/detection_engine/rules/api.ts

 /**
- * Patch provided Rule
+ * Patch provided rule


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.

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.

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.

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.

madirey · 2020-08-14T20:11:00Z

x-pack/plugins/security_solution/public/detections/containers/detection_engine/rules/api.ts

+    signal,
+  });
+
+const createRuleWithValidation = async ({ rule, signal }: CreateRulesProps): Promise<RulesSchema> =>


Is the idea that we're validating on the FE and on the BE? Does that buy us anything to be able to validate from the UI?

@peluja1012 Any opinions on this? IMO it's simpler, more readable, and more maintainable if we only validate in one place (the backend APIs).

We still get the benefit of using common types on both API and UI code... that buys us a lot in terms of static type consistency. Once we start doing dynamic validation though, I feel like we're starting to hit the other side of the cost/benefit curve.

I decided to remove these here for now. I think you bring up a lot of valid points that can be addressed separately if we decide to go down that route.

yctercero · 2020-09-03T15:22:15Z

x-pack/plugins/security_solution/public/detections/containers/detection_engine/rules/mock.ts

@@ -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 = {


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

madirey · 2020-09-03T17:59:05Z

...ck/plugins/security_solution/public/detections/containers/detection_engine/rules/api.test.ts

+      fetchMock.mockResolvedValue(getRulesSchemaMock());
+    });
+
+    test('POSTs rule', async () => {


The test name here says POST, but the test is checking for PUT

madirey

I pulled this down and ran through several test cases... created rules, updated rules, sorted detection alerts in various configurations... everything looked good!

yctercero · 2020-09-04T12:08:17Z

@elasticmachine merge upstream

kibanamachine · 2020-09-04T13:58:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3656316

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
securitySolution	1948	+1	1947

async chunks size

id	value	diff	baseline
securitySolution	9.9MB	+7.4KB	9.9MB

page load bundle size

id	value	diff	baseline
securitySolution	812.9KB	-89.0B	813.0KB

distributable file count

id	value	diff	baseline
total	45464	+2	45462

History

💚 Build #72395 succeeded 6393862
💚 Build #72341 succeeded 5e1fd07
💔 Build #72301 failed 0b3773d
💔 Build #72115 failed f713a28
💚 Build #68159 succeeded 79ab71d

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

…ts types (elastic#75009) ## Summary Part of the DE tech debt includes moving to use the common io-ts types in the UI. Currently, we are using some of them in some places and other times we're using the front end defined typescript types. This means that changes often have to be done on both ends, validation isn't being done at the UI boundary on request or response. Using the common types could help us avoid bugs moving forward. Obviously, there's a lot of code to touch so trying to just do it piece by piece. This PR addresses the following: - Replaced uses of `NewRule` with common type `CreateRuleSchema` and `UpdateRuleSchema`. These were being combined a bit - Split `usePersistRule` which was used for both `POST` and `PUT` into two hooks - `useCreateRule` and `useUpdateRule`. - The logic for combining these two relied on checking for the existence of an `id` - if present it would `PUT`, otherwise it would `POST`. - However, `id` is not a required property for either of these, so it's not reliable - Updated the rule edit flow to use `useUpdateRule` - Updated the rule creation flow to use `useCreateRule`

* master: (47 commits) Do not require id & description when creating a logstash pipeline (elastic#76616) Remove commented src/core/tsconfig file (elastic#76792) Replaced whitelistedHosts with allowedHosts in actions ascii docs (elastic#76731) [Dashboard First] Genericize Attribute Service (elastic#76057) [ci-metrics] unify distributable file count metrics (elastic#76448) [Security Solution][Detections] Handle conflicts on alert status update (elastic#75492) [eslint] convert to @typescript-eslint/no-unused-expressions (elastic#76471) [DOCS] Add default time range filter to advanced settings (elastic#76414) [Security Solution] Refactor NetworkTopNFlow to use Search Strategy (elastic#76249) [Dashboard] Update Index Patterns when Child Index Patterns Change (elastic#76356) [ML] Add option to Advanced Settings to set default time range filter for AD jobs (elastic#76347) Add CSM app to CODEOWNERS (elastic#76793) [Security Solution][Exceptions] - Updates exception item find sort field (elastic#76685) [Security Solution][Detections][Tech Debt] - Move to using common io-ts types (elastic#75009) [Lens] Drag dimension to replace (elastic#75895) URI encode the index names we fetch in the fetchIndices lib function. (elastic#76584) [Security Solution] Resolver retrieve entity id of documents without field mapped (elastic#76562) [Ingest Manager] validate agent route using AJV instead kbn-config-schema (elastic#76546) Updated non-dev usages of node-forge (elastic#76699) [Ingest Pipelines] Processor forms for processors K-S (elastic#75638) ...

…ts types (#75009) (#76780) ## Summary Part of the DE tech debt includes moving to use the common io-ts types in the UI. Currently, we are using some of them in some places and other times we're using the front end defined typescript types. This means that changes often have to be done on both ends, validation isn't being done at the UI boundary on request or response. Using the common types could help us avoid bugs moving forward. Obviously, there's a lot of code to touch so trying to just do it piece by piece. This PR addresses the following: - Replaced uses of `NewRule` with common type `CreateRuleSchema` and `UpdateRuleSchema`. These were being combined a bit - Split `usePersistRule` which was used for both `POST` and `PUT` into two hooks - `useCreateRule` and `useUpdateRule`. - The logic for combining these two relied on checking for the existence of an `id` - if present it would `PUT`, otherwise it would `POST`. - However, `id` is not a required property for either of these, so it's not reliable - Updated the rule edit flow to use `useUpdateRule` - Updated the rule creation flow to use `useCreateRule`

elasticmachine · 2021-09-23T14:27:02Z

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

yctercero added 3 commits August 13, 2020 14:47

wip using shared types, NewRule

8548c1d

cleaned up types

4e77ecf

updated some rule hooks and tests

9a7b05c

yctercero commented Aug 13, 2020

View reviewed changes

yctercero commented Aug 14, 2020

View reviewed changes

yctercero self-assigned this Aug 14, 2020

yctercero added release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.10.0 v8.0.0 Feature:Detection Rules Security Solution rules and Detection Engine labels Aug 14, 2020

yctercero marked this pull request as ready for review August 14, 2020 00:19

Merge branch 'master' of github.com:yctercero/kibana into ui-types

79ab71d

yctercero requested review from a team as code owners August 14, 2020 14:35

madirey reviewed Aug 14, 2020

View reviewed changes

yctercero added 5 commits September 2, 2020 17:07

Merge branch 'master' of github.com:yctercero/kibana into ui-types

dcfb598

removed api io-ts validation, can be discussed separatly

f713a28

cleanup

6a9f432

Merge branch 'master' of github.com:yctercero/kibana into ui-types

f983e83

update missing mock in unit test

0b3773d

yctercero commented Sep 3, 2020

View reviewed changes

update unit test

5e1fd07

madirey reviewed Sep 3, 2020

View reviewed changes

update test description

6393862

madirey approved these changes Sep 3, 2020

View reviewed changes

Merge branch 'master' into ui-types

3656316

yctercero merged commit d2156b7 into elastic:master Sep 4, 2020

yctercero mentioned this pull request Sep 4, 2020

[7.x] [Security Solution][Detections][Tech Debt] - Move to using common io-ts types (#75009) #76780

Merged

yctercero deleted the ui-types branch October 14, 2020 11:59

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections][Tech Debt] - Move to using common io-ts types #75009

[Security Solution][Detections][Tech Debt] - Move to using common io-ts types #75009

yctercero commented Aug 13, 2020 •

edited

Loading

yctercero Aug 13, 2020

yctercero Aug 14, 2020

elasticmachine commented Aug 14, 2020

madirey Aug 14, 2020

yctercero Aug 15, 2020

madirey Aug 15, 2020

yctercero Sep 3, 2020

madirey Aug 14, 2020 •

edited

Loading

madirey Aug 15, 2020

madirey Aug 15, 2020

yctercero Sep 3, 2020

yctercero Sep 3, 2020

madirey Sep 3, 2020

madirey left a comment

yctercero commented Sep 4, 2020

kibanamachine commented Sep 4, 2020

elasticmachine commented Sep 23, 2021

[Security Solution][Detections][Tech Debt] - Move to using common io-ts types #75009

[Security Solution][Detections][Tech Debt] - Move to using common io-ts types #75009

Conversation

yctercero commented Aug 13, 2020 • edited Loading

Summary

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madirey Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madirey left a comment

Choose a reason for hiding this comment

yctercero commented Sep 4, 2020

kibanamachine commented Sep 4, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

distributable file count

History

elasticmachine commented Sep 23, 2021

yctercero commented Aug 13, 2020 •

edited

Loading

madirey Aug 14, 2020 •

edited

Loading