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

[Alerting] Control Alerts Management via feature controls & privileges #72029

Merged
merged 210 commits into from
Jul 28, 2020

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Jul 16, 2020

Summary

Closes #69665

This PR removes the alerting and actions ui privileges (alerting:show, actions:show, etc...) and instead relies on the standard Kibana feature control model to decide whether management displays the Alerts Management section under management.

Dev Notes

If you want your plugin to grant a user access to the Alerts Management section you need to specify it under management in your feature configuration.

You do so by adding this sections:

management: {
    insightsAndAlerting: ['triggersActions'],
},

There are 3 places where you can specify it:

  1. Directly on the feature. When security is disabled, this grants access to every role who's granted access to the Feature via Feature Controls. When security is enabled, this specifies that the feature has access to this management section and is required before you can grant this to a specific role.
  2. Under the all privilege. When security is enabled, this grants access to every role who's granted the all privilege to the Feature via Feature Controls
  3. Under the read privilege. When security is enabled, this grants access to every role who's granted the read privilege to the Feature via Feature Controls

This means that, likely, you'll have to specify this in 3 places in your plugin to cover all 3 scenarios. This is more verbose than what we had before, but it is the correct way to do it (to align with the rest of Kibana) and it means the Triggers and Actions plugin no longer needs to know about each plugin that wants to gain access (which means we can more easily support future alerting usage).

Checklist

Delete any items that are not applicable to this PR.

For maintainers

gmmorris added 30 commits May 21, 2020 09:50
…bana into alerting/consumer-based-rbac

* 'alerting/consumer-based-rbac' of github.com:gmmorris/kibana:
  added rbac in alerting
  fixed unit test
  fixed typing, commented unused authz
  made SO client unsecure in alerting
* master: (26 commits)
  [Console]remove completion for type for filter queries and aggs (elastic#68103)
  [ML] Transforms: Filter aggregation support (elastic#67591)
  [ES UI Shared] Monaco XJSON (elastic#67485)
  [Index Management] Add data streams functionality to indices tab (elastic#67940)
  [Discover] Fix renaming of saved search not displayed in breadcrumb (elastic#67577)
  [SECURITY] Rename siem plugin to security_solution (elastic#67902)
  [Uptime] Fix Telemetry Api flaky test (elastic#67358)
  [Data plugin] Add configuration property to enable / disable autocomplete (elastic#67847)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  ...
* master: (57 commits)
  Add app arch team as owner of datemath package (elastic#66880)
  [Observability] Landing page for Observability (elastic#67467)
  [SIEM] Fix timeline buildGlobalQuery (elastic#68320)
  Optimize saved objects getScopedClient and HTTP API (elastic#68221)
  [Maps] Fix mb-style interpolate style rule (elastic#68413)
  update script to always download node (elastic#68421)
  [SECURITY SOLEIL] Fix selection of event type when no siem index signal created (elastic#68291)
  [DOCS] Adds note about configuring File Data Visualizer (elastic#68407)
  [DOCS] Adds link from remote clusters to index patterns (elastic#68406)
  [QA] slack notify on failure (elastic#68126)
  upgrade eslint-plugin-react-hooks from 2.3.0 to 4.0.4 (elastic#68295)
  moving to jira to a gold license (elastic#67178)
  [DOCS] Revises doc on adding data (elastic#68038)
  [APM] Add ThemeProvider to support dark mode (elastic#68242)
  Make welcome screen disabling first action in loginIfPrompted (elastic#68238)
  [QA] Code coverage: unskip tests, collect tests results, exclude bundles from report (elastic#64477)
  [ML] Functional tests - disable flaky regression and classification creation test
  [Alerting] change eventLog ILM requests to absolute URLs (elastic#68331)
  Report page load asset size (elastic#66224)
  [SIEM][CASE] Change SIEM to Security (elastic#68365)
  ...
* master:
  [ML] DFAnalytics results: ensure ml result fields are shown in data grid (elastic#68305)
  [security_solution] enable react-hooks/exhaustive-deps (elastic#68470)
  Closes elastic#66867 by adding missing, requried API params (elastic#68465)
  [Telemetry] collect number of visualization saved in the past 7, 30 and 90 days (elastic#67865)
  [Logs UI] View in context tweaks (elastic#67777)
  Kibana developer examples landing page (elastic#67049)
  Bump decompress package version (elastic#68386)
  fix elastic#66185 (elastic#66186)
  Bump pdfmake package version (elastic#68395)
  Unskip embeddables/adding_children suite (elastic#68111)
  Add embed mode options in the Share UI (elastic#58435)
  Adding key to avoid react warning (elastic#68491)
* master: (37 commits)
  [DOCS] Adds documentation for drilldowns (elastic#68122)
  [Telemetry] telemetry.sendUsageFrom: "server" by default (elastic#68071)
  [ML] Transforms: Support sub-aggregations (elastic#68306)
  Fixed pre-configured docs link points to the wrong page and functional tests configs (elastic#68606)
  [Ingest Manager] Update queries from `stream.*` to `dataset.*` (elastic#68322)
  Enable Watcher by default to fix bug in which Watcher doesn't render in the side nav (elastic#68602)
  Convert Index Templates API routes to snakecase. (elastic#68463)
  [SECURITY SOLUTION] [Detections] Add / Update e2e tests to ensure initial rule runs are successful (elastic#68441)
  [Ingest] OpenAPI spec file (elastic#68323)
  chore(NA): skip apis Endpoint plugin Endpoint policy api (elastic#68638)
  bumping makelogs version to v6.0.0 (elastic#66163)
  [SIEM] Add create template button (elastic#66613)
  Bump websocket-extensions from 0.1.3 to 0.1.4 (elastic#68414)
  [ML] Sample data modules - use event.dataset instead of index name (elastic#68538)
  [ML] Functional tests - fix job validation API test with maxModelMemoryLimit (elastic#68501)
  [ML] Functional tests - stabilize DFA job creation (elastic#68495)
  [TSVB] Allows the user to change the tooltip mode (elastic#67775)
  chore(NA): skip apis Endpoint plugin Endpoint alert API when data is in elasticsearch (elastic#68613)
  chore(NA): skip endpoint Endpoint Alert Page: when es has data and user has navigated to the page (elastic#68596)
  [SIEM][Detection Engine] Converts from joi to use io-ts and moves the types to common  (elastic#68127)
  ...
gmmorris added 3 commits July 21, 2020 18:20
* master:
  [Security Solution][Timeline] Add Empty view to the Timelines page (elastic#72576)
  [Security Solution][Resolver] Show process detail panel when clicking a process node (elastic#72563)
  Move manifest packageConfig mocks into security_solution plugin (elastic#72527)
  [QA][Code Coverage] Fixup Team Assignment (elastic#72467)
  [docs] remove references to tile map visualization in supported aggregations (elastic#72493)
  [ci][apm-ui] fix argument name for disabling pr comments (elastic#72633)
  Only check that the event ids are the same in arrays (elastic#72624)
  Add doc titles to ES UI apps (elastic#71045)
  Add Upgrade Assistant API integration test to ensure the reindex operation saved object can handle immense error messages (elastic#72347)
  [APM] Disable flaky rum e2e’s (elastic#72614)
  Applying tiny fix from 72532 to main branch (elastic#72533)
  [APM] Update script with new roles/users (elastic#72599)
  [Security Solution] Add margin (elastic#72542)
  Migrated fixed_scroll karma tests to jest (elastic#72258)
  [ML] Handling data recognizer saved object errors (elastic#72447)
  [Monitoring] Fix the messaging around needing TLS enabled (elastic#72310)
  [Task Manager] Batches the update operations in Task Manager  (elastic#71470)
…feature-privileges

* alerting/consumer-based-rbac:
  [Security Solution][Timeline] Add Empty view to the Timelines page (elastic#72576)
  [Security Solution][Resolver] Show process detail panel when clicking a process node (elastic#72563)
  renamed variable to make it clear the SO client is unsecured
  Move manifest packageConfig mocks into security_solution plugin (elastic#72527)
  [QA][Code Coverage] Fixup Team Assignment (elastic#72467)
  [docs] remove references to tile map visualization in supported aggregations (elastic#72493)
  [ci][apm-ui] fix argument name for disabling pr comments (elastic#72633)
  Only check that the event ids are the same in arrays (elastic#72624)
  includes hidden params type in SO client
  Add doc titles to ES UI apps (elastic#71045)
  Add Upgrade Assistant API integration test to ensure the reindex operation saved object can handle immense error messages (elastic#72347)
  [APM] Disable flaky rum e2e’s (elastic#72614)
  Applying tiny fix from 72532 to main branch (elastic#72533)
  [APM] Update script with new roles/users (elastic#72599)
  [Security Solution] Add margin (elastic#72542)
  Migrated fixed_scroll karma tests to jest (elastic#72258)
  [ML] Handling data recognizer saved object errors (elastic#72447)
  [Monitoring] Fix the messaging around needing TLS enabled (elastic#72310)
  [Task Manager] Batches the update operations in Task Manager  (elastic#71470)
* master: (34 commits)
  Adds Role Based Access-Control to the Alerting & Action plugins based on Kibana Feature Controls (elastic#67157)
  [Monitoring] Revert direct shipping code (elastic#72505)
  Use server basepath  when creating reporting jobs (elastic#72722)
  Adding api test for transaction_groups /breakdown and /avg_duration_by_browser (elastic#72623)
  [Task Manager] Addresses flaky test introduced by buffered store (elastic#72815)
  [Observability] filter "hasData" api by processor event (elastic#72810)
  do  not pass title as part of tsvb request (elastic#72619)
  [Lens] Legend config (elastic#70619)
  Stabilize closing toast (elastic#72097)
  stabilize failing test (elastic#72086)
  Stabilize filter bar test (elastic#72032)
  Unskip vislib tests (elastic#71452)
  [ML] Fix layout of anomaly chart tooltip for long field values (elastic#72689)
  fix preAuth/preRouting mocks (elastic#72663)
  [Security Solution] Hide KQL bar (all pages) and alerts filters (Detections) when Resolver is full screen (elastic#72788)
  [Uptime] Rename Whitelist to Allowlist in parse_filter_map (elastic#71584)
  [Security Solution] Fixes exception modal not loading content (elastic#72770)
  [Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items (elastic#72748)
  [Detections] Add validation for Threshold value field (elastic#72611)
  [SIEM][Detection Engine][Lists] Adds version and immutability data structures (elastic#72730)
  ...
@gmmorris gmmorris marked this pull request as ready for review July 22, 2020 14:08
@gmmorris gmmorris requested review from a team as code owners July 22, 2020 14:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jul 22, 2020
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

code LGTM, but also want to load it up and try it live, and haven't yet. Leaving as a comment for now, till I do (today or tomorrow).

@@ -129,6 +129,16 @@ export class AlertingPlugin {
this.spaces = plugins.spaces?.spacesService;
this.security = plugins.security;

core.capabilities.registerProvider(() => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious - we don't have one of these stanzas for actions, but I'm guessing we can only registerProvider() in one place. What happens if security is enabled, alerts is disabled and actions is enabled? Thinking about a future-Kibana that uses actions in other scenarios besides alerts ...

Copy link
Contributor Author

@gmmorris gmmorris Jul 23, 2020

Choose a reason for hiding this comment

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

Oh, actually, this might be an oversight.
@legrego do we still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I've checked and removing this causes it to appear to unprivileged users.
I think this PR might not be ready yet and might require some work by Security.

Copy link
Member

@legrego legrego Jul 28, 2020

Choose a reason for hiding this comment

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

Thanks for the ping - we still want this in place, at least for the time being. There is a bug in Feature Controls where features can't successfully "register" their management sections, because the features plugin isn't accounting for them when building the canonical set of Capabilities. The workaround is for plugins to provide this capability manually, until we can update the Features plugin.

This provider does not influence or control any sort of access management. It merely defines the boolean flag (with the correct default value of true) so that it may be controlled by Security/Spaces as needed.

FWIW, we can remove this once #67791 merges, as this PR updates the features plugin to respect management sections.

});
setAlertsState({ ...alertsState, isLoading: false });
const hasAnyAuthorizedAlertType = alertTypesState.data.size > 0;
if (hasAnyAuthorizedAlertType) {
Copy link
Member

Choose a reason for hiding this comment

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

curious there's no else clause for this if. This function appears to just update state after loading the alerts, and that doesn't need to be done if there's nothing to view. But then wondering if the user will end up at with an empty view, or perhaps even gulp a view which suggests to create a new alert :-). Guessing this case is covered already, just not clear from looking just at this function, will test it live.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, it's handled in the render itself - which is why I would like us to split this file down into like 4 or 5 files - it's way too big and hard to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that can be a separate PR

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

APM changes lgtm

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.

LGTM. Tested live for different users with variety of alert/action role settings, and everything work as expected.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

loaded up the PR and took it for a spin - works as advertised

@gmmorris
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
triggers_actions_ui 151 -1 152

async chunks size

id value diff baseline
triggers_actions_ui 725.8KB +12.2KB 713.6KB

page load bundle size

id value diff baseline
triggers_actions_ui 262.8KB -4.1KB 266.9KB

History

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

Copy link
Contributor

@justinkambic justinkambic 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

I pulled this branch and ran it, seemed to work ok 👌

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

infra plugin changes LGTM

@gmmorris gmmorris merged commit f410474 into elastic:master Jul 28, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 28, 2020
elastic#72029)

This PR removes the alerting and actions ui privileges (alerting:show, actions:show, etc...) and instead relies on the standard Kibana feature control model to decide whether management displays the Alerts Management section under management.
gmmorris added a commit that referenced this pull request Jul 28, 2020
#72029) (#73457)

This PR removes the alerting and actions ui privileges (alerting:show, actions:show, etc...) and instead relies on the standard Kibana feature control model to decide whether management displays the Alerts Management section under management.
@gchaps
Copy link
Contributor

gchaps commented Oct 20, 2020

@gmmorris For future PRs with the release_note:plulgin_api_changes, please use the title "Dev Docs" instead of "Dev Notes" in the summary. Otherwise, the script won't pick up your write-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:APM All issues that need APM UI Team support Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alerting appears under management even when users can't use them