[Security Solution] Add missing Exceptions API OpenAPI specifications #185951
+4,564
−1,046
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maximpn
added
release_note:skip
maximpn
force-pushed
the
add-missing-exceptions-api-oas
branch
2 times, most recently
from
22d2fc8
to
61ad063
Compare
maximpn
force-pushed
the
add-missing-exceptions-api-oas
branch
11 times, most recently
from
d4d7cd8
to
09f4a03
Compare
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
maximpn
requested review from
banderror and
marshallmain
and removed request for
rylnd
maximpn
force-pushed
the
add-missing-exceptions-api-oas
branch
from
aeb4a00
to
d51ce47
Compare
maximpn
force-pushed
the
add-missing-exceptions-api-oas
branch
2 times, most recently
from
4de0de3
to
632c69a
Compare
Files by Code Ownerelastic/kibana-operations
elastic/security-defend-workflows
elastic/security-detection-engine
elastic/security-detection-rule-management
elastic/security-solution
|
16 tasks
jbudz
approved these changes
Jun 24, 2024
paul-tavares
approved these changes
Jun 24, 2024
maximpn
force-pushed
the
add-missing-exceptions-api-oas
branch
from
5268734
to
f64d0b8
Compare
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @maximpn |
e40pud
reviewed
Jul 1, 2024
| export const ExceptionListTypeEnum = ExceptionListType.enum; | ||
|
|
||
| export type ExceptionListName = z.infer<typeof ExceptionListName>; | ||
| export const ExceptionListName = NonEmptyString; |
There was a problem hiding this comment.
Non empty string is not required by the old schema packages/kbn-securitysolution-io-ts-list-types/src/common/name/index.ts, so this will be a breaking change.
export const name = t.string;
export type Name = t.TypeOf<typeof name>;
|
|
||
| export type CreateExceptionListRequestBody = z.infer<typeof CreateExceptionListRequestBody>; | ||
| export const CreateExceptionListRequestBody = z.object({ | ||
| list_id: ExceptionListHumanId.optional(), |
There was a problem hiding this comment.
According to the old schema, we would check if the input is null and create a uuid in that case, otherwise we would expect non empty string. I do not see the parity in behaviour here. Is it possible to do the same via openapi and zod?
/**
* Types the DefaultUuid as:
* - If null or undefined, then a default string uuidv4() will be
* created otherwise it will be checked just against an empty string
*/
export const DefaultUuid = new t.Type<string, string | undefined, unknown>(
'DefaultUuid',
t.string.is,
(input, context): Either<t.Errors, string> =>
input == null ? t.success(uuidv4()) : NonEmptyString.validate(input, context),
t.identity
);
There was a problem hiding this comment.
Should we add .default(uuidv4())?
| export const ExceptionNamespaceTypeEnum = ExceptionNamespaceType.enum; | ||
|
|
||
| export type ExceptionListTags = z.infer<typeof ExceptionListTags>; | ||
| export const ExceptionListTags = z.array(NonEmptyString); |
There was a problem hiding this comment.
I do not see the requirement for non empty string for this type in old version.
export const tags = DefaultStringArray;
where
export const DefaultStringArray = new t.Type<string[], string[] | undefined, unknown>(
'DefaultStringArray',
t.array(t.string).is,
(input, context): Either<t.Errors, string[]> =>
input == null ? t.success([]) : t.array(t.string).validate(input, context),
t.identity
);
| export const ExceptionListOsTypeArray = z.array(ExceptionListOsType); | ||
|
|
||
| export type ExceptionListVersion = z.infer<typeof ExceptionListVersion>; | ||
| export const ExceptionListVersion = z.number().int().min(1); |
There was a problem hiding this comment.
In old schema we use PositiveIntegerGreaterThanZero type for the version. Should we add check for that as well?
| export type CreateExceptionListItemRequestBody = z.infer<typeof CreateExceptionListItemRequestBody>; | ||
| export const CreateExceptionListItemRequestBody = z.object({ | ||
| item_id: ExceptionListItemHumanId.optional(), | ||
| list_id: ExceptionListHumanId, |
There was a problem hiding this comment.
same as in case with exception list schema, do we need to add .default(uuidv4()) here?
| - `exception-list-agnostic`: Specify an exception list that is shared across spaces. | ||
|
|
||
| */ | ||
| filter: FindExceptionListsFilter.optional(), |
There was a problem hiding this comment.
in packages/kbn-securitysolution-io-ts-list-types/src/common/filter/index.ts non empty string for filter is not required
| /** | ||
| * The page number to return | ||
| */ | ||
| page: z.coerce.number().int().min(0).optional(), |
There was a problem hiding this comment.
According to packages/kbn-securitysolution-io-ts-types/src/string_to_positive_number/index.ts both page and per_page should be positive numbers. We use StringToPositiveNumber type which does failure check stringAsNumber <= 0. Can we use z.number().positive() here?
There was a problem hiding this comment.
Same in other files where we use these types.
| /** | ||
| * Determines which field is used to sort the results | ||
| */ | ||
| sort_field: NonEmptyString.optional(), |
There was a problem hiding this comment.
Old schema does not require non empty string for sort_field
| export type FindExceptionListsResponse = z.infer<typeof FindExceptionListsResponse>; | ||
| export const FindExceptionListsResponse = z.object({ | ||
| data: z.array(ExceptionList), | ||
| page: z.number().int().min(0), |
There was a problem hiding this comment.
All three page, per_page and total marked with // TODO: Change this out for PositiveNumber from siem. Maybe we should add .positive() to these types instead of .min(0).
| } | ||
|
|
||
| return response.ok({ body: DeleteExceptionListItemResponse.parse(deleted) }); |
There was a problem hiding this comment.
I see that before we would return 500 error if response validation fails. Is this still the same in case of zod parse?
|
@maximpn do these changes require going in 8.15 or can it wait for 8.16? cc @banderror |
|
@yctercero We don't have to merge this before the FF - we can take our time to review this. I updated the labels. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #183837
Summary
This PR adds missing OpenAPI specifications for Exceptions API which are the following
POST /api/exception_lists/_exportPOST /api/exception_lists/_importPOST /api/exception_listsGET /api/exception_listsPUT /api/exception_listsDELETE /api/exception_listsGET /api/exception_lists/_findPOST /api/exception_lists/_duplicatePOST /api/exception_lists/itemsGET /api/exception_lists/itemsPUT /api/exception_lists/itemsDELETE /api/exception_lists/itemsGET /api/exception_lists/items/_findGET /api/exception_lists/summaryPOST /api/exceptions/sharedPOST /api/detection_engine/rules/{id}/exceptions