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

[Connectors] moves the connector deprecation check to the server side #130541

Merged

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Apr 19, 2022

Summary

This PR moves the deprecation check for connectors to the server side.

This is groundwork needed to address an issue raised by this cases PR: #130372
With Cases supporting PreConfigured connectors again they need to be able to identify deprecated connectors even when they are PreConfigured.

This change is required as the deprecation check for ServiceNow relies on the configuration of the connector, and exposing this configuration to users can be problematic from a security perspective. While this is relatively safe with regular connectors, as we can perform a privilege check for these, it's less secure for preconfigured connectors, as these are not space aware.

To address this this PR:

  1. Moved the deprecation check to the server side
  2. Exposes the result of this check on all APIs that retried connectors using a new is_deprecated field on APIs such as get, getAll, create etc.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@gmmorris gmmorris added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Actions/ConnectorsManagement Issues related to Connectors Management UX labels Apr 19, 2022
@gmmorris gmmorris marked this pull request as ready for review April 21, 2022 12:11
@gmmorris gmmorris requested review from a team as code owners April 21, 2022 12:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@gmmorris gmmorris changed the title moved depreciation check to the server side [Connectors] moves the connector depreciation check to the server side Apr 21, 2022
Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

API docs changes LGTM!

@gmmorris gmmorris changed the title [Connectors] moves the connector depreciation check to the server side [Connectors] moves the connector deprecation check to the server side Apr 21, 2022
@gmmorris
Copy link
Contributor Author

@elasticmachine merge upstream

@gmmorris gmmorris added enhancement New value added to drive a business result release_note:enhancement and removed enhancement New value added to drive a business result labels Apr 21, 2022
@gmmorris
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine kibanamachine requested a review from a team as a code owner April 22, 2022 11:56
@gmmorris
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Uptime changes LGTM !!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
actions 127 128 +1

Async chunks

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

id before after diff
synthetics 783.7KB 783.7KB +31.0B
triggersActionsUi 711.7KB 712.4KB +750.0B
total +781.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
triggersActionsUi 100.2KB 100.0KB -169.0B
Unknown metric groups

API count

id before after diff
actions 127 128 +1

History

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

@@ -146,6 +149,7 @@ describe('updateActionRoute', () => {
name: 'My name',
config: { foo: true },
isPreconfigured: false,
isDeprecated: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test at least one of the routes with a isDeprecated: true result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we have a few of those in the end-to-end tests I didn't think it was critical to have every possible output covered in the unit test as well at route level (it is tested on the client itself).

Thanks Ersin!

Copy link
Contributor

@ersin-erdal ersin-erdal left a comment

Choose a reason for hiding this comment

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

If we think that a positive test case is not required in for some test cases the rest LGTM :)

export const isConnectorDeprecated = (
connector: RawAction | ConnectorWithOptionalDeprecation
): boolean => {
if (connector.actionTypeId === '.servicenow' || connector.actionTypeId === '.servicenow-sir') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need to worry about .servicenow-itom?

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 don't know - I just moved the existing check to the server side.
@cnasikas ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Christos is on PTO, so I'll leave that enhancement to another time (seeing as this is the existing functionality on main).

Thanks for the feedback @ymao1

Copy link
Member

Choose a reason for hiding this comment

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

Thank you @gmmorris for this PR 🚀 ! .servicenow-itom does not use an application so we do not have to worry about it.

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Security Solution changes LGTM

@gmmorris gmmorris merged commit eb043f7 into elastic:main Apr 26, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 26, 2022
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
…elastic#130541)

This PR fixes a UI glitch where preconfigured connectors couldn't show a deprecation warning on SN connectors as the check on the client side couldn't support them.
@gmmorris gmmorris added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Actions/ConnectorsManagement Issues related to Connectors Management UX release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.3.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

10 participants