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

Jump to bottom

[Security Solution][Endpoint] Change Trusted Apps to use item_id as its identifier and Enable Trusted Apps filtering by id in the UI #115276

Merged
merged 17 commits into from
Oct 19, 2021
Merged

[Security Solution][Endpoint] Change Trusted Apps to use item_id as its identifier and Enable Trusted Apps filtering by id in the UI #115276

merged 17 commits into from
Oct 19, 2021

Conversation

paul-tavares
Copy link
Contributor

Summary

  • Changes server side trusted apps services to use the ExceptionListeItemSchema[item_id] property value as the Trusted App identifier in order to better support API filtering
  • Changes the "View full details" action of the Policy Details Trusted App card actions to use the filter by Trusted App id when redirecting the user to the Trusted Apps list

NOTE: The API changes remain backwards compatible in that the id provided via the API call will first be used to retrieve the existing trusted app using the item_id field, and if it is not found, then the id field is used. API consumer may find this a bit weird as the ID returned in the response from the API might differ from the ID used as the input param.

olm-1800-change-trusted-app-id-to-item-id

Checklist

@paul-tavares paul-tavares added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution auto-backport Deprecated - use backport:version if exact versions are needed OLM Sprint v7.16.0 labels Oct 15, 2021
@paul-tavares paul-tavares self-assigned this Oct 15, 2021
@paul-tavares paul-tavares marked this pull request as ready for review October 18, 2021 13:30
@paul-tavares paul-tavares requested a review from a team as a code owner October 18, 2021 13:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

@paul-tavares
Copy link
Contributor Author

getting this out of Draft so I can get reviews on it. still not sure it will be merged - I want to think a little more about this change and if there are any impacts that i have not yet thought of. I will also fix the UT, but code is there

@paul-tavares
Merge remote-tracking branch 'upstream/master' into task/olm-1800-all…
be7b619 
…ow-trusted-apps-list-filter-by-ta-id
@paul-tavares
Merge remote-tracking branch 'upstream/master' into task/olm-1800-all…
bc9c58c 
…ow-trusted-apps-list-filter-by-ta-id

# Conflicts:
#	x-pack/plugins/security_solution/public/management/pages/policy/view/trusted_apps/list/policy_trusted_apps_list.tsx
@paul-tavares
re-apply change to view all details action link
6cdd7dc
dasansol92
dasansol92 reviewed Oct 18, 2021
View reviewed changes
x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/service.ts
): Promise<ExceptionListItemSchema | null> => {
const trustedAppExceptionItem = await exceptionsListClient.getExceptionListItem({
itemId: id,
id: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is. The prop is defined as "required", and the value could be undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

got it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing to note that I learned with these changes - don't set id to '' because any value that is not undefined (string, even if empty) will take precedence over itemId if defined. That had me debugging for a bit :)

pzl
pzl approved these changes Oct 18, 2021
View reviewed changes
Copy link
Member

@pzl pzl left a comment

Choose a reason for hiding this comment

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

🐑 🎸

ashokaditya
ashokaditya approved these changes Oct 18, 2021
View reviewed changes
Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

🚀 🐑

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/service.ts Outdated
exceptionsListClient: ExceptionListClient,
{ id }: DeleteTrustedAppsRequestParams
): Promise<void> => {
const trustedAppExceptionItem = await findTrustedAppExceptionItemItemByIdOrItemId(
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: typo? ...ItemItem... should be perhaps ...ByIdOrItemId? 🤔

@paul-tavares paul-tavares enabled auto-merge (squash) October 18, 2021 15:43
@paul-tavares
Copy link
Contributor Author

@elasticmachine merge upstream

@kevinlog
Copy link
Contributor

@elasticmachine merge upstream

@kevinlog
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 4.6MB 4.6MB +2.0B

History

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

cc @paul-tavares

@paul-tavares paul-tavares merged commit 533e5d8 into elastic:master Oct 19, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 19, 2021
@paul-tavares @kibanamachine
[Security Solution][Endpoint] Change Trusted Apps to use item_id as…
52ca0f8 
… its identifier and Enable Trusted Apps filtering by id in the UI (elastic#115276)

* Add `item_id` to list of searchable fields
* trusted apps api changes to use `item_id` instead of SO `id`
* Change Policy Details Trusted App "View all details" action URL to show TA list filtered by the TA id
@kibanamachine kibanamachine mentioned this pull request Oct 19, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 19, 2021
@kibanamachine @paul-tavares
[Security Solution][Endpoint] Change Trusted Apps to use item_id as…
65786cf 
… its identifier and Enable Trusted Apps filtering by id in the UI (#115276) (#115509)

* Add `item_id` to list of searchable fields
* trusted apps api changes to use `item_id` instead of SO `id`
* Change Policy Details Trusted App "View all details" action URL to show TA list filtered by the TA id

Co-authored-by: Paul Tavares <[email protected]>
@paul-tavares paul-tavares deleted the task/olm-1800-allow-trusted-apps-list-filter-by-ta-id branch October 19, 2021 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pzl pzl pzl approved these changes

@ashokaditya ashokaditya ashokaditya approved these changes

@dasansol92 dasansol92 dasansol92 approved these changes

Assignees

@paul-tavares paul-tavares

Labels
auto-backport Deprecated - use backport:version if exact versions are needed OLM Sprint release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@paul-tavares @elasticmachine @kevinlog @kibanamachine @pzl @ashokaditya @dasansol92