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

[ResponseOps][Rules] Remove unintended internal Find routes API with public access #193757

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Sep 23, 2024

Summary

Fixes #192957

Removes the internal/_find route from public access by moving the hard-coded options into the route builder functions.

@Zacqary Zacqary added release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v9.0.0 Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework v8.16.0 labels Sep 23, 2024
@Zacqary Zacqary requested a review from a team as a code owner September 23, 2024 16:47
@elasticmachine
Copy link
Contributor

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

@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@Zacqary Zacqary added the backport:version Backport to applied version labels label Sep 23, 2024
@Zacqary
Copy link
Contributor Author

Zacqary commented Sep 23, 2024

@elasticmachine merge upstream

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Could you please add a unit test (x-pack/plugins/alerting/server/routes/rule/apis/find/find_rules_route.test.ts) to be sure we register the correct routes with the correct access?

@@ -37,15 +38,12 @@ const buildFindRulesRoute = ({
router,
excludeFromPublicApi = false,
usageCounter,
options: routerOptions,
}: BuildFindRulesRouteParams) => {
router.get(
Copy link
Member

Choose a reason for hiding this comment

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

This call to the router will also be done when calling findInternalRulesRoute. This will register the GET /internal/alerting/rules/_find which we do not want. Should we add a check like

if (path !== INTERNAL_ALERTING_API_FIND_RULES_PATH) { /// rouer.get }

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restructured the if statement to be an if...else to handle this

@cnasikas cnasikas added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:version Backport to applied version labels labels Sep 24, 2024
@Zacqary Zacqary requested a review from a team as a code owner September 25, 2024 17:24
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Sep 25, 2024
@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 25, 2024

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] x-pack/test/alerting_api_integration/security_and_spaces/group1/config.ts / alerting api integration security and spaces enabled Alerts - Group 1 alerts find with post internal superuser at space1 should filter out types that the user is not authorized to get retaining pagination
  • [job] [logs] x-pack/test/alerting_api_integration/security_and_spaces/group1/config.ts / alerting api integration security and spaces enabled Alerts - Group 1 alerts find with post internal superuser at space1 should filter out types that the user is not authorized to get retaining pagination
  • [job] [logs] FTR Configs #55 / Alerting create should create rules with mapped parameters
  • [job] [logs] FTR Configs #55 / Alerting create should create rules with mapped parameters
  • [job] [logs] FTR Configs #84 / Alerting update handle update alert request appropriately should handle update alert request appropriately
  • [job] [logs] FTR Configs #84 / Alerting update handle update alert request appropriately should handle update alert request appropriately
  • [job] [logs] FTR Configs #66 / ObservabilityApp Custom threshold rule saved the rule correctly
  • [job] [logs] FTR Configs #66 / ObservabilityApp Custom threshold rule saved the rule correctly

Metrics [docs]

✅ unchanged

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ResponseOps][Rules] Remove unintended internal Find routes API with public access
5 participants