[EDR Workflows][Artifact transfer 4] UI fine tuning#257983
[EDR Workflows][Artifact transfer 4] UI fine tuning#257983gergoabraham merged 18 commits intoelastic:mainfrom
Conversation
df53b14 to
e05b80a
Compare
prerequisites: - #247967 - #247976 follow-up: - #257983 closes #250470 ## Summary > [!note] > Hidden behind feature flag (as part of the Endpoint exception move effort): > ``` > xpack.securitySolution.enableExperimental: > - endpointExceptionsMovedUnderManagement > ``` This PR allows importing all Endpoint artifacts! Lot's of changes, here's a summary written by Claude, plus some additional information: This PR updates the Endpoint artifact import validator registered for the `POST /api/exception_lists/_import` API. The changes are gated behind the `endpointExceptionsMovedUnderManagement` feature flag. ### What's changed **Import validation overhaul (`getExceptionsPreImportHandler`):** - Replaces the old simple "block all non-endpoint-exception artifacts" logic with full per-artifact-type validation support - When the feature flag is enabled, dispatches validation to the appropriate artifact-specific validator (`TrustedAppValidator`, `BlocklistValidator`, `EventFilterValidator`, `HostIsolationExceptionsValidator`, `TrustedDeviceValidator`, `EndpointExceptionsValidator`) - Supports importing only **one** artifact list type per import call (returns 400 if multiple types are mixed) - Handles `overwrite` semantics by deleting visible items before import (respecting user permissions), then disabling the Lists API's own overwrite to avoid full-list deletion **Space-aware validation:** - Each imported item is validated against the user's space permissions: non-global artifact management users can only import items owned by the current space; users with global artifact management privilege can import items from other spaces, provided those items are visible in the current space - Invalid owner space IDs are rejected _(this means error);_ invalid policy IDs are stripped and noted in a comment on the item **On error handling:** - if there's a general problem (like no user access), the request is rejected. - if there's a per-item problem, the request succeeds, but it'll contain the errors per item, which errors we can later show to the user **Data enrichment on import:** - Adds the `imported_artifact` tag to all imported items - Adds an audit comment to each item with the original author and creation date - Adds the `ownerSpaceId` tag to items that lack one (backward compatibility for pre-space-awareness exports) **`ExceptionsListPreImportServerExtension` type change:** - The extension point's data type changed from `PromiseFromStreams` → `{ data: PromiseFromStreams; overwrite: boolean }`, allowing the extension to inspect and override the `overwrite` flag **`ExceptionListClient.bulkDeleteExceptionListItems`:** - New public method for bulk-deleting exception list items (replaces the previous slow per-item sequential deletion via `asyncForEach`) **`ExceptionItemImportError` class:** - New error class implementing `BulkErrorErrorSchema`, used to surface per-item validation errors in the import response (instead of aborting the entire import) **Tests:** - Comprehensive new FTR integration test suite (`artifact_import.ts`) covering privilege checks, space-awareness scenarios, invalid data handling, overwrite behavior, comment/tag enrichment, and backward compatibility ## Testing To ease testing, I added some ugly details (58fcfb3) to the success toast, but this is not something that will stay. ## Follow-up PR possibilities There are some things we probably also need to add, but I'd like to keep them out of this PR. I can think of these: - improve UI feedback to user on errors (the toast doesn't look ideal) - do not allow any artifact type be imported on any artifact pages and rule exceptions page. this is now allowed, since the API endpoint is the same. (we could fix this e.g. by adding a new query parameter to the import API, or by adding a new endpoint artifact import API that calls the lists import service) Follow-up PR is already in-progress: - #257983 ## Screenshots <img width="965" height="497" alt="image" src="https://github.com/user-attachments/assets/9a552242-48b0-45f2-979b-eadacf73ffcc" /> <img width="975" height="497" alt="image" src="https://github.com/user-attachments/assets/73d77a99-c2c6-421b-a22d-2c2aeb595b90" /> <img width="1227" height="747" alt="image" src="https://github.com/user-attachments/assets/3d3b77f2-0077-441d-b4eb-5519fcda25a5" /> ### Some error messages <img width="973" height="500" alt="image" src="https://github.com/user-attachments/assets/b751a6cb-e84a-44df-8d47-c676a20253ce" /> Response: ```json { "message": "EndpointArtifactError: Importing multiple Endpoint artifact exception list types at the same time is not supported", "status_code": 400 } ``` <img width="1231" height="745" alt="image" src="https://github.com/user-attachments/assets/ef4d9032-ecef-4f32-9c80-241ecdc1a978" /> ☝️ probably won't be like this, it's just to visualize the response for this PR. will be improved in a follow-up pr Response: ```json { "errors": [ { "error": { "status_code": 403, "message": "EndpointArtifactError: Importing artifacts with invalid owner space IDs is not allowed. The following space ID is invalid or unaccessible by current user: nope" }, "list_id": "endpoint_host_isolation_exceptions", "item_id": "5cc4585e-edbb-445f-a0d6-a6a67bb29d04" }, { "error": { "status_code": 403, "message": "EndpointArtifactError: Endpoint authorization failure. Importing artifacts that are not visible in the current space is not allowed" }, "list_id": "endpoint_host_isolation_exceptions", "item_id": "4c791b4a-5e87-4eb3-9713-1c80f6c98b80" } ], "success": false, "success_count": 1, "success_exception_lists": true, "success_count_exception_lists": 0, "success_exception_list_items": false, "success_count_exception_list_items": 1 } ``` ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…8046) prerequisites: - elastic#247967 - elastic#247976 follow-up: - elastic#257983 closes elastic#250470 ## Summary > [!note] > Hidden behind feature flag (as part of the Endpoint exception move effort): > ``` > xpack.securitySolution.enableExperimental: > - endpointExceptionsMovedUnderManagement > ``` This PR allows importing all Endpoint artifacts! Lot's of changes, here's a summary written by Claude, plus some additional information: This PR updates the Endpoint artifact import validator registered for the `POST /api/exception_lists/_import` API. The changes are gated behind the `endpointExceptionsMovedUnderManagement` feature flag. ### What's changed **Import validation overhaul (`getExceptionsPreImportHandler`):** - Replaces the old simple "block all non-endpoint-exception artifacts" logic with full per-artifact-type validation support - When the feature flag is enabled, dispatches validation to the appropriate artifact-specific validator (`TrustedAppValidator`, `BlocklistValidator`, `EventFilterValidator`, `HostIsolationExceptionsValidator`, `TrustedDeviceValidator`, `EndpointExceptionsValidator`) - Supports importing only **one** artifact list type per import call (returns 400 if multiple types are mixed) - Handles `overwrite` semantics by deleting visible items before import (respecting user permissions), then disabling the Lists API's own overwrite to avoid full-list deletion **Space-aware validation:** - Each imported item is validated against the user's space permissions: non-global artifact management users can only import items owned by the current space; users with global artifact management privilege can import items from other spaces, provided those items are visible in the current space - Invalid owner space IDs are rejected _(this means error);_ invalid policy IDs are stripped and noted in a comment on the item **On error handling:** - if there's a general problem (like no user access), the request is rejected. - if there's a per-item problem, the request succeeds, but it'll contain the errors per item, which errors we can later show to the user **Data enrichment on import:** - Adds the `imported_artifact` tag to all imported items - Adds an audit comment to each item with the original author and creation date - Adds the `ownerSpaceId` tag to items that lack one (backward compatibility for pre-space-awareness exports) **`ExceptionsListPreImportServerExtension` type change:** - The extension point's data type changed from `PromiseFromStreams` → `{ data: PromiseFromStreams; overwrite: boolean }`, allowing the extension to inspect and override the `overwrite` flag **`ExceptionListClient.bulkDeleteExceptionListItems`:** - New public method for bulk-deleting exception list items (replaces the previous slow per-item sequential deletion via `asyncForEach`) **`ExceptionItemImportError` class:** - New error class implementing `BulkErrorErrorSchema`, used to surface per-item validation errors in the import response (instead of aborting the entire import) **Tests:** - Comprehensive new FTR integration test suite (`artifact_import.ts`) covering privilege checks, space-awareness scenarios, invalid data handling, overwrite behavior, comment/tag enrichment, and backward compatibility ## Testing To ease testing, I added some ugly details (58fcfb3) to the success toast, but this is not something that will stay. ## Follow-up PR possibilities There are some things we probably also need to add, but I'd like to keep them out of this PR. I can think of these: - improve UI feedback to user on errors (the toast doesn't look ideal) - do not allow any artifact type be imported on any artifact pages and rule exceptions page. this is now allowed, since the API endpoint is the same. (we could fix this e.g. by adding a new query parameter to the import API, or by adding a new endpoint artifact import API that calls the lists import service) Follow-up PR is already in-progress: - elastic#257983 ## Screenshots <img width="965" height="497" alt="image" src="https://github.com/user-attachments/assets/9a552242-48b0-45f2-979b-eadacf73ffcc" /> <img width="975" height="497" alt="image" src="https://github.com/user-attachments/assets/73d77a99-c2c6-421b-a22d-2c2aeb595b90" /> <img width="1227" height="747" alt="image" src="https://github.com/user-attachments/assets/3d3b77f2-0077-441d-b4eb-5519fcda25a5" /> ### Some error messages <img width="973" height="500" alt="image" src="https://github.com/user-attachments/assets/b751a6cb-e84a-44df-8d47-c676a20253ce" /> Response: ```json { "message": "EndpointArtifactError: Importing multiple Endpoint artifact exception list types at the same time is not supported", "status_code": 400 } ``` <img width="1231" height="745" alt="image" src="https://github.com/user-attachments/assets/ef4d9032-ecef-4f32-9c80-241ecdc1a978" /> ☝️ probably won't be like this, it's just to visualize the response for this PR. will be improved in a follow-up pr Response: ```json { "errors": [ { "error": { "status_code": 403, "message": "EndpointArtifactError: Importing artifacts with invalid owner space IDs is not allowed. The following space ID is invalid or unaccessible by current user: nope" }, "list_id": "endpoint_host_isolation_exceptions", "item_id": "5cc4585e-edbb-445f-a0d6-a6a67bb29d04" }, { "error": { "status_code": 403, "message": "EndpointArtifactError: Endpoint authorization failure. Importing artifacts that are not visible in the current space is not allowed" }, "list_id": "endpoint_host_isolation_exceptions", "item_id": "4c791b4a-5e87-4eb3-9713-1c80f6c98b80" } ], "success": false, "success_count": 1, "success_exception_lists": true, "success_count_exception_lists": 0, "success_exception_list_items": false, "success_count_exception_list_items": 1 } ``` ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
3624ffa to
d69858c
Compare
…8046) prerequisites: - elastic#247967 - elastic#247976 follow-up: - elastic#257983 closes elastic#250470 ## Summary > [!note] > Hidden behind feature flag (as part of the Endpoint exception move effort): > ``` > xpack.securitySolution.enableExperimental: > - endpointExceptionsMovedUnderManagement > ``` This PR allows importing all Endpoint artifacts! Lot's of changes, here's a summary written by Claude, plus some additional information: This PR updates the Endpoint artifact import validator registered for the `POST /api/exception_lists/_import` API. The changes are gated behind the `endpointExceptionsMovedUnderManagement` feature flag. ### What's changed **Import validation overhaul (`getExceptionsPreImportHandler`):** - Replaces the old simple "block all non-endpoint-exception artifacts" logic with full per-artifact-type validation support - When the feature flag is enabled, dispatches validation to the appropriate artifact-specific validator (`TrustedAppValidator`, `BlocklistValidator`, `EventFilterValidator`, `HostIsolationExceptionsValidator`, `TrustedDeviceValidator`, `EndpointExceptionsValidator`) - Supports importing only **one** artifact list type per import call (returns 400 if multiple types are mixed) - Handles `overwrite` semantics by deleting visible items before import (respecting user permissions), then disabling the Lists API's own overwrite to avoid full-list deletion **Space-aware validation:** - Each imported item is validated against the user's space permissions: non-global artifact management users can only import items owned by the current space; users with global artifact management privilege can import items from other spaces, provided those items are visible in the current space - Invalid owner space IDs are rejected _(this means error);_ invalid policy IDs are stripped and noted in a comment on the item **On error handling:** - if there's a general problem (like no user access), the request is rejected. - if there's a per-item problem, the request succeeds, but it'll contain the errors per item, which errors we can later show to the user **Data enrichment on import:** - Adds the `imported_artifact` tag to all imported items - Adds an audit comment to each item with the original author and creation date - Adds the `ownerSpaceId` tag to items that lack one (backward compatibility for pre-space-awareness exports) **`ExceptionsListPreImportServerExtension` type change:** - The extension point's data type changed from `PromiseFromStreams` → `{ data: PromiseFromStreams; overwrite: boolean }`, allowing the extension to inspect and override the `overwrite` flag **`ExceptionListClient.bulkDeleteExceptionListItems`:** - New public method for bulk-deleting exception list items (replaces the previous slow per-item sequential deletion via `asyncForEach`) **`ExceptionItemImportError` class:** - New error class implementing `BulkErrorErrorSchema`, used to surface per-item validation errors in the import response (instead of aborting the entire import) **Tests:** - Comprehensive new FTR integration test suite (`artifact_import.ts`) covering privilege checks, space-awareness scenarios, invalid data handling, overwrite behavior, comment/tag enrichment, and backward compatibility ## Testing To ease testing, I added some ugly details (58fcfb3) to the success toast, but this is not something that will stay. ## Follow-up PR possibilities There are some things we probably also need to add, but I'd like to keep them out of this PR. I can think of these: - improve UI feedback to user on errors (the toast doesn't look ideal) - do not allow any artifact type be imported on any artifact pages and rule exceptions page. this is now allowed, since the API endpoint is the same. (we could fix this e.g. by adding a new query parameter to the import API, or by adding a new endpoint artifact import API that calls the lists import service) Follow-up PR is already in-progress: - elastic#257983 ## Screenshots <img width="965" height="497" alt="image" src="https://github.com/user-attachments/assets/9a552242-48b0-45f2-979b-eadacf73ffcc" /> <img width="975" height="497" alt="image" src="https://github.com/user-attachments/assets/73d77a99-c2c6-421b-a22d-2c2aeb595b90" /> <img width="1227" height="747" alt="image" src="https://github.com/user-attachments/assets/3d3b77f2-0077-441d-b4eb-5519fcda25a5" /> ### Some error messages <img width="973" height="500" alt="image" src="https://github.com/user-attachments/assets/b751a6cb-e84a-44df-8d47-c676a20253ce" /> Response: ```json { "message": "EndpointArtifactError: Importing multiple Endpoint artifact exception list types at the same time is not supported", "status_code": 400 } ``` <img width="1231" height="745" alt="image" src="https://github.com/user-attachments/assets/ef4d9032-ecef-4f32-9c80-241ecdc1a978" /> ☝️ probably won't be like this, it's just to visualize the response for this PR. will be improved in a follow-up pr Response: ```json { "errors": [ { "error": { "status_code": 403, "message": "EndpointArtifactError: Importing artifacts with invalid owner space IDs is not allowed. The following space ID is invalid or unaccessible by current user: nope" }, "list_id": "endpoint_host_isolation_exceptions", "item_id": "5cc4585e-edbb-445f-a0d6-a6a67bb29d04" }, { "error": { "status_code": 403, "message": "EndpointArtifactError: Endpoint authorization failure. Importing artifacts that are not visible in the current space is not allowed" }, "list_id": "endpoint_host_isolation_exceptions", "item_id": "4c791b4a-5e87-4eb3-9713-1c80f6c98b80" } ], "success": false, "success_count": 1, "success_exception_lists": true, "success_count_exception_lists": 0, "success_exception_list_items": false, "success_count_exception_list_items": 1 } ``` ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
41d06a6 to
e81dd46
Compare
for easier cleanup when we remove the FF
e81dd46 to
90047a5
Compare
|
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
szwarckonrad
left a comment
There was a problem hiding this comment.
Tested manually with FF endpointExceptionsMovedUnderManagement enabled:
✅ Updated wording: Verified import/export labels no longer use "list" on Trusted Applications and Blocklist pages
✅ Shared Exception Lists page: Uploading an endpoint artifact file (trusted apps) shows error toast blocking the import
✅ Wrong artifact type rejection: Uploading a blocklists file on the Trusted Applications page shows danger toast after confirmation
✅ Confirmation modal flow: Modal appears on import, Cancel closes modal but keeps flyout open with file selected, re-clicking Import reopens modal
✅ Successful import: Importing correct file shows success toast ("Artifacts imported" / "All artifacts were imported successfully."), flyout closes, list refreshes
✅ Append behavior: Re-importing the same file does not wipe existing artifacts; duplicates are skipped
✅ Danger toast + errors modal: Re-importing duplicates shows danger toast with "View errors" button; errors modal shows numbered list with correct item IDs
✅ Warning toast + errors modal: Importing a file with one valid item and one item with invalid ownerSpaceId shows warning toast with counts and "View errors" button; errors modal shows the failed item with correct message ("This artifact can't be imported because it belongs to a space you don't have access to"); EndpointArtifactError: prefix properly stripped
✅ Loading state: Both flyout and modal Import buttons disabled during request
Great work!
| const errorsToDisplay: Array<{ itemId: string; message: string }> = useMemo( | ||
| () => | ||
| errors.map((error) => ({ | ||
| itemId: error.item_id ?? 'undefined', |
There was a problem hiding this comment.
When item_id is missing, the modal shows item (undefined) which reads a bit rough. Consider something like "Unknown item" instead.
There was a problem hiding this comment.
good point. but, wouldn't that read not so well with the numbered list?
1. item (xxx-yyy-zzz): blah blah
2. unknown item: blah blah
3. item (yyy-zzz-xxx): blah blah
or we can just go maybe with unknown instead of undefined:
1. item (xxx-yyy-zzz): blah blah
2. item (unknown): blah blah
3. item (yyy-zzz-xxx): blah blah
@natasha-moore-elastic, what would you suggest?
natasha-moore-elastic
left a comment
There was a problem hiding this comment.
Approving to unblock but left some minor suggestions, thanks!
| 'xpack.securitySolution.exceptionsTable.importEndpointArtifactsErrorText', | ||
| { | ||
| defaultMessage: | ||
| 'On this page only shared exception lists can be imported, but at least one file contains Endpoint artifacts. Endpoint artifacts can be imported on their respective pages.', |
There was a problem hiding this comment.
| 'On this page only shared exception lists can be imported, but at least one file contains Endpoint artifacts. Endpoint artifacts can be imported on their respective pages.', | |
| 'You can only import shared exception lists here, but at least one of the imported files contains endpoint artifacts. Import endpoint artifacts from their dedicated pages instead.', |
There was a problem hiding this comment.
thanks for the suggestions, updated all the texts here:
365d6b9
paul-tavares
left a comment
There was a problem hiding this comment.
Did code review only and left some comments. I'm approving as most of those can be addressed (if deemed necessary) later
| * @returns {Promise<Set<string>>} set of list ids found in the file | ||
| */ | ||
| export const parseListIdsFromImportedFile = async (file: File): Promise<Set<string>> => | ||
| (await file.text()) |
There was a problem hiding this comment.
Is this a good idea? To load the entire file into memory in the browser?
My suggestions if you really do need to go through the file in the browser:
- Consider using a more efficient method for reading file content -
File#stream() - Should this entire process use
AbortController? so that if the user navigates away from the page while this is ongoing, you can efficiently abort?
IMO - this type of check should happen on the server at the API level and not in the browser. Files could be large
There was a problem hiding this comment.
on browser vs server: I agree, however, as we only have one API for both shared exception list pages and endpoint artifact pages, and that API now accepts shared exception lists and endpoint exceptions, blocking endpoint exceptions on it (e.g. by adding a query parameter) would be a breaking change. I added this quick check to avoid that.
on performance: as far as i understood, on modern browsers this shouldn't be an issue. import API allows a maximum of 10k items to be imported, one item is 600-800 byte, depending on the type, so calculating with 1kB per item is still 10MB, not much.
created a test file 13k items, sized 8.6MB, it took 30-45ms on my oldish macbook both in Chrome and Firefox, with the following result anyway:

so for now, I think we should be safe with this approach on this feature, that's probably not used extensively in daily routine of the users, without adding more complexity by using streams
| data-test-subj="importExceptionListCreateNewCheckbox" | ||
| checked={asNewList} | ||
| disabled={endpointListImporting} | ||
| onChange={(e) => { |
There was a problem hiding this comment.
Consider memoizing this callback
| ) : ( | ||
| <EuiToolTip | ||
| position="bottom" | ||
| content={endpointListImporting ? i18n.IMPORT_EXCEPTION_ENDPOINT_LIST_WARNING : ''} |
There was a problem hiding this comment.
FYI: I find it so confusing to use i18n as an object of messages when by default we use it as the i18n library function
There was a problem hiding this comment.
i sort of agree, but this file (and a lot of others not under the 'management' folder) uses this approach 🤷
| content={endpointListImporting ? i18n.IMPORT_EXCEPTION_ENDPOINT_LIST_WARNING : ''} | ||
| > | ||
| <EuiCheckbox | ||
| id={'createNewListCheckbox'} |
There was a problem hiding this comment.
I think this is ok here only because this component will only ever be displayed on the page once, but normally - you would want to assign HTML id's using EUI's useGeneratedHtmlId () hook to ensure no collision
| <EuiText size="s">{text}</EuiText> | ||
|
|
||
| <EuiFlexGroup justifyContent="flexEnd" direction="row"> | ||
| <EuiButton size="s" onClick={() => onShowErrors(errors)}> |
There was a problem hiding this comment.
Consider memoizing this onClick callback
| (error) => | ||
| !( | ||
| error.error.status_code === 409 && | ||
| error.error.message.match(/Found that list_id: "\w+" already exists/) |
There was a problem hiding this comment.
looking for matches in error messages returned from the server is a bit brittle and the UI can break unexpectedly if that message is changed in the server. One way to ensure this condition is caught is to add an FTR/Cypress tests that validates this condition
There was a problem hiding this comment.
yes, that's a good point, thanks for bringing it up. i'm planning to add some extra coverage in the PR that enables the FF, i'll add a test to cover this.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
History
|
prerequisites:
Summary
Note
Hidden behind feature flag (as part of the Endpoint exception move effort):
Limit which lists can be imported on which page
You can only import shared exception lists here, but at least one of the imported files contains endpoint artifacts. Import endpoint artifacts from their dedicated pages instead.You can only import blocklist entries here.You can only import Endpoint exceptions here.You can only import event filters here.Keep existing artifacts on import
Artifact import now APPENDS imported artifacts to existing ones, instead of OVERWRITING the whole list
Positive friction for importing Endpoint artifacts
Import artifacts to your artifact list.This will add new artifacts to your list. If an artifact you're importing already exists, the existing version will be kept, and the import of that artifact will be skipped.Display import errors in a better way, following new design
Artifacts imported/All artifacts were imported successfullyImport completed with errors/{importedCount} imported, {failedCount} failed. Review the errors for details.Artifacts weren't imported/The artifacts couldn't be imported. Review the errors and try again.Import errorsSome items couldn't be imported. Review the errors below for details.Server error responses are updated with new texts
This artifact can't be imported because you don't have permission to manage artifacts in other spaces. Contact your administrator for access.This artifact can't be imported because you don't have permission to manage global artifacts. Contact your administrator for access.This artifact can't be imported because it isn't visible in the current space. Try importing it from a matching space or a space with access to the related policy.This artifact can't be imported because it belongs to a space you don't have access to. Update the artifact in its original space and try again.Screenshots
Import limitation
Shared lists page
An endpoint artifact page
New texts
etc.
Confirm modal
Displaying the results on toasts
Success
Some items have errors
All items have errors
Results modal
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.