[RAM] Update RBAC to manage es-query/generic o11y threshold rule #165182

XavierM · 2023-08-30T00:38:33Z

Summary

This PR is here to update the alerting authorization model to allow .es-query and observability.rules.threshold to work with different consumers. We also the rule find's API to allow to filter on consumers. We update the alert client from the rule_registry plugin to get the alert index through the rule type and the alerting plugin like we did for the search strategy.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…bana into o11y-rbac-rule-feature-branch

…-ref HEAD~1..HEAD --fix'

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

mikecote

@XavierM can you list the four testing scenarios / rules you used yesterday? I want to use them when reviewing the alerting_authorization.ts file.

x-pack/plugins/observability/common/constants.ts

x-pack/plugins/alerting/server/rule_type_registry.ts

x-pack/plugins/alerting/server/rule_type_registry_deprecated_consumers.ts

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

XavierM · 2023-09-01T02:00:34Z

@mikecote I will write jest test around this four scenario so it is easy to understand.

elasticmachine · 2023-09-02T01:39:26Z

Pinging @elastic/apm-ui (Team:APM)

elasticmachine · 2023-09-02T01:39:26Z

Pinging @elastic/uptime (Team:uptime)

…fix'

sorenlouv

apm changes lgtm

…bana into o11y-rbac-rule-feature-branch

…fix'

…bana into o11y-rbac-rule-feature-branch

XavierM · 2023-09-06T18:41:26Z

src/plugins/discover/public/application/main/components/top_nav/open_alerts_popover.tsx

@@ -84,7 +85,7 @@ export function AlertsPopover({

    return triggersActionsUi?.getAddRuleFlyout({
      metadata: discoverMetadata,
-      consumer: 'discover',


About this change we created an issue to the breaking changes committee -> https://github.com/elastic/dev/issues/2344

JiaweiWu

LGTM, pretty straight forward logic.

Have some nitpicks mostly, not blocking though.

JiaweiWu · 2023-09-06T18:48:49Z

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

@@ -232,7 +242,8 @@ export class AlertingAuthorization {
  public async getAuthorizationFilter(
    authorizationEntity: AlertingAuthorizationEntity,
    filterOpts: AlertingAuthorizationFilterOpts,
-    operation: WriteOperations | ReadOperations
+    operation: WriteOperations | ReadOperations,
+    featuresIds?: Set<string>


nitpick: does this have to be a set? seems like an array is enough

we do not want duplicated of feature ids

maybe its better to dedupe in the function and let it accept arrays, I think it makes the interface a bit friendlier to work with

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

JiaweiWu · 2023-09-06T18:55:23Z

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

+              if (!isEmpty(ruleTypeAuth.validLegacyConsumers)) {
+                ruleTypeAuth.validLegacyConsumers.forEach((consumer) => {
+                  if (consumer === ALERTS_FEATURE_ID || isEmpty(featuresIds)) {
+                    if (!allPossibleConsumers[consumer]) {


do we have to do this check? can we just set it regardless

I think you are right that we do not really need the check with our use cases but it feel safer for some reason.

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

JiaweiWu · 2023-09-06T18:58:39Z

x-pack/plugins/alerting/server/routes/find_rules.ts

@@ -47,6 +47,7 @@ const querySchema = schema.object({
  ),
  fields: schema.maybe(schema.arrayOf(schema.string())),
  filter: schema.maybe(schema.string()),
+  filter_consumers: schema.maybe(schema.arrayOf(schema.string())),


should we validate specifically against consumers instead of just strings?

I was wondering too to be honest let me think more

x-pack/plugins/rule_registry/server/routes/get_browser_fields_by_feature_id.ts

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

ymao1 · 2023-09-06T20:38:39Z

x-pack/plugins/alerting/server/rule_type_registry_deprecated_consumers.ts

+  'apm.anomaly': [ALERTS_FEATURE_ID],
+  'apm.error_rate': [ALERTS_FEATURE_ID],
+  'apm.transaction_error_rate': [ALERTS_FEATURE_ID],
+  'test.always-firing': [ALERTS_FEATURE_ID],


Feels weird to be keeping this list. WDYT about having the ruleType specify something like legacyConsumer: boolean | string[] where legacyConsumer: true resolves to setting validLegacyConsumers: [ALERTS_FEATURE_ID] on registration and legacyConsumer: ['discover'] resolves to validLegacyConsumers: [ALERTS_FEATURE_ID, 'discover'] on registration?

That's a decision that @mikecote and I took to avoid to updating every rule type in the repo and also good catch I need to add some unit test so we know if someone is updating this list or not. Also, if we add it to the rule type, it will have to be a conditional type, it is not really safe.

ymao1 · 2023-09-06T20:41:11Z

x-pack/plugins/alerting/server/rules_client/methods/aggregate.ts


  let authorizationTuple;
  try {
    authorizationTuple = await context.authorization.getFindAuthorizationFilter(
      AlertingAuthorizationEntity.Rule,
-      alertingAuthorizationFilterOpts
+      alertingAuthorizationFilterOpts,
+      isEmpty(filterConsumers) ? undefined : new Set(filterConsumers)


Does this respect RBAC? API users shouldn't be able to pass whatever set of consumers they want to aggregate over, they should just be able to limit which consumers to aggregate over that they already have access to correct?

Yes it does follow RBAC, thanks for scaring me here ;) but we do, we are just saying that we only care about some consumers. It is helping us to only show o11y rules in the rule management page.

kibana-ci · 2023-09-07T21:56:02Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #7 / Discover alerting Search source Alert should navigate to alert results via view in app link
[job] [logs] FTR Configs #7 / Discover alerting Search source Alert should navigate to alert results via view in app link

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [f83694a]

History

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

XavierM added 2 commits August 29, 2023 16:44

update RBAC to work with es-query and the new o11y threshold

cb9f3a6

Merge branch 'o11y-rbac-rule-feature-branch' of github.com:XavierM/ki…

bef9184

…bana into o11y-rbac-rule-feature-branch

XavierM added release_note:skip Skip the PR/issue when compiling release notes v8.11.0 labels Aug 30, 2023

XavierM requested review from a team as code owners August 30, 2023 00:38

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

4aa1e7c

…-ref HEAD~1..HEAD --fix'

XavierM commented Aug 30, 2023

View reviewed changes

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts Outdated Show resolved Hide resolved

pgayvallet approved these changes Aug 30, 2023

View reviewed changes

mikecote reviewed Aug 30, 2023

View reviewed changes

renaming + update API to make sure everything is working as expected

86136a2

XavierM added 3 commits September 1, 2023 11:20

fix types + jest tests

f6d81b6

update discover feature test since we remove the alerting

2ec1033

fix types + rmv discover + rmv alert from o11y

0cef337

XavierM requested review from a team as code owners September 2, 2023 01:39

botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Sep 2, 2023

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

6f92f34

…fix'

sorenlouv approved these changes Sep 3, 2023

View reviewed changes

XavierM added 2 commits September 5, 2023 13:45

fix tests

62c2101

Merge branch 'o11y-rbac-rule-feature-branch' of github.com:XavierM/ki…

c014a6c

…bana into o11y-rbac-rule-feature-branch

XavierM requested a review from a team as a code owner September 5, 2023 17:47

shahzad31 approved these changes Sep 5, 2023

View reviewed changes

XavierM and others added 6 commits September 5, 2023 14:45

rmv expect error

4c5ec3b

Merge branch 'o11y-rbac-rule-feature-branch' of github.com:elastic/ki…

3ef8b24

…bana into o11y-rbac-rule-feature-branch

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

6c929f2

…fix'

add authorization test to match reality

2e4754d

Merge branch 'o11y-rbac-rule-feature-branch' of github.com:XavierM/ki…

74f978e

…bana into o11y-rbac-rule-feature-branch

fix FTR

d301128

XavierM commented Sep 6, 2023

View reviewed changes

JiaweiWu approved these changes Sep 6, 2023

View reviewed changes

review I

4338874

ymao1 reviewed Sep 6, 2023

View reviewed changes

XavierM added 6 commits September 6, 2023 17:13

review II

c9a67d5

FTR II

351c7cc

Fix FTR rule type

5e846c9

fix rule types FTR tests

2d518ec

fix tests

cea4459

fix types

4f0eab1

XavierM merged commit 7337d16 into elastic:o11y-rbac-rule-feature-branch Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RAM] Update RBAC to manage es-query/generic o11y threshold rule #165182

[RAM] Update RBAC to manage es-query/generic o11y threshold rule #165182

XavierM commented Aug 30, 2023 •

edited

Loading

mikecote left a comment

XavierM commented Sep 1, 2023 •

edited

Loading

elasticmachine commented Sep 2, 2023

elasticmachine commented Sep 2, 2023

sorenlouv left a comment

XavierM Sep 6, 2023

JiaweiWu left a comment

JiaweiWu Sep 6, 2023

XavierM Sep 6, 2023

JiaweiWu Sep 6, 2023

JiaweiWu Sep 6, 2023

XavierM Sep 6, 2023

JiaweiWu Sep 6, 2023

XavierM Sep 6, 2023

ymao1 Sep 6, 2023

XavierM Sep 6, 2023 •

edited

Loading

ymao1 Sep 6, 2023

XavierM Sep 6, 2023 •

edited

Loading

kibana-ci commented Sep 7, 2023 •

edited

Loading

[RAM] Update RBAC to manage es-query/generic o11y threshold rule #165182

[RAM] Update RBAC to manage es-query/generic o11y threshold rule #165182

Conversation

XavierM commented Aug 30, 2023 • edited Loading

Summary

Checklist

mikecote left a comment

Choose a reason for hiding this comment

XavierM commented Sep 1, 2023 • edited Loading

elasticmachine commented Sep 2, 2023

elasticmachine commented Sep 2, 2023

sorenlouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaweiWu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XavierM Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XavierM Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Sep 7, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

History

XavierM commented Aug 30, 2023 •

edited

Loading

XavierM commented Sep 1, 2023 •

edited

Loading

XavierM Sep 6, 2023 •

edited

Loading

XavierM Sep 6, 2023 •

edited

Loading

kibana-ci commented Sep 7, 2023 •

edited

Loading