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][Alerts] Fix stack alerts plugin API and page permissions #191110

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

Conversation

umbopepato
Copy link
Member

@umbopepato umbopepato commented Aug 22, 2024

Summary

  • Adds the rac api privilege to stackAlerts plugin's Kibana feature registration (done in [ResponseOps][Alerting] Fix stackAlerts plugin missing rac API auth scope #193948)
  • Extends the fetchAlertsIndexNames API to return a boolean flag that indicates if the end user (not Kibana user) has (at least) read index privileges on the alert index name returned by the API, regardless of the alerting authorization
  • Uses the flag mentioned in the previous point to only render the alert filter controls bar if the user has personal read access to the indices targeted by the current feature ids
  • Only uses feature ids the user has access to for the stack alerts page (shouldn't be necessary, but it restricts the resulting index pattern, which is helpful to better identify when to show/hide the filter controls)
  • Fixes a small issue that caused useAlertsDataView to return an intermediate empty data view object while loading

To verify

  1. Create rules that fire alerts in Stack management, Observability and Security
  2. Wait for alerts to be created
  3. Check that the stack alerts page renders correctly and shows alerts coherent with the feature filter and other KQL bar filters
  4. Create a role with only Stack Management > Rules : Read privilege
  5. Create a user with that role
  6. In another window, open Kibana with the newly created user
  7. Check that the Stack Management > Alerts page renders correctly, only showing alerts from stackAlerts rule types and without the filter controls bar
  8. Add read privilege to the .alerts-* index to the role
  9. Reload the alerts page as unprivileged user and check that the filter controls are now visible
  10. Gradually add other solution permissions and check that the alerts shown in the page change accordingly

References

Fixes #186451

Checklist

Delete any items that are not applicable to this PR.

@umbopepato umbopepato added release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 labels Aug 22, 2024
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the 186451-fix-stack-alerts-page-permissions branch from 4532beb to e57eca2 Compare August 27, 2024 07:34
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the 186451-fix-stack-alerts-page-permissions branch from e57eca2 to 2ede3c6 Compare August 27, 2024 08:30
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the 186451-fix-stack-alerts-page-permissions branch from 2ede3c6 to e504d0e Compare August 27, 2024 10:21
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the 186451-fix-stack-alerts-page-permissions branch from e504d0e to d163ced Compare August 27, 2024 14:10
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the 186451-fix-stack-alerts-page-permissions branch from d163ced to d37fb0c Compare August 27, 2024 14:17
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato force-pushed the 186451-fix-stack-alerts-page-permissions branch from d37fb0c to c0ca606 Compare August 27, 2024 14:48
@umbopepato
Copy link
Member Author

/ci

@umbopepato umbopepato marked this pull request as ready for review August 27, 2024 16:19
@umbopepato umbopepato requested review from a team as code owners August 27, 2024 16:19
@elasticmachine
Copy link
Contributor

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

/**
* Indicates if the end user has read access to the index, besides the alerting authorization
*/
hasReadIndexPrivilege?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

question about this field, shouldn't we do this implicitly on the backend based on the request user? Seems a bit odd to expose a field like this to the client.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main use case is authorizing views like the filter controls bar that are unaware of the alerting RBAC and directly work with the indices. In these cases we don't have a way to know if the end user (besides their alerting permissions) has direct read access to the alerting indices. Since this is particularly useful for unprivileged users, checking the index permissions directly on the client seems a bit fragile: the user should have access to the plugin(s) that expose the privilege check functionality, which is unlikely in this case.
I could be missing a more straightforward way to check this so please if you have any ideas let me know 🙂

Copy link
Contributor

@JiaweiWu JiaweiWu Sep 3, 2024

Choose a reason for hiding this comment

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

Right but this means anyone could "spoof" the read index privilege by passing this field? At that point you might as well just always assume the user has privilege

In these cases we don't have a way to know if the end user (besides their alerting permissions) has direct read access to the alerting indices.

Since we have the request, we'll have the user, can't we check the user's index privilege? unless we're someone making a request on behalf of another user or something.

the user should have access to the plugin(s) that expose the privilege check functionality

Then how does the client know of the user's index privilege but not the server? The client had to get it's data from somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right but this means anyone could "spoof" the read index privilege by passing this field? At that point you might as well just always assume the user has privilege

Not sure I follow you here. This is set by the server, not the client, (I assume) similarly to how the capabilities object informs the client on what the user has access to for example.

Since we have the request, we'll have the user, can't we check the user's index privilege? unless we're someone making a request on behalf of another user or something.

This privilege is not instrumental to authenticating the alerts/_index request itsefl, but to tell the client if the user has access to the ES alerts index itself. Unprivileged users might not have a direct access but still be able to indirectly access them through the alerting APIs.

The controls embeddable thought works directly with the ES indices, not through the alerting APIs. That's why we need to check if the user has "direct" access to those indices: to hide the controls and avoid 403 errors.

Copy link
Member Author

@umbopepato umbopepato Sep 4, 2024

Choose a reason for hiding this comment

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

If you create a Role with only Security read capabilities without access to the indices and try to open the Security > Alerts page, you'll see a missing authn prompt that pretty much exemplifies this situation. We'd have to do something similar with the controls embeddable

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #75 / Reporting Functional Tests with Security enabled Access to Management > Reporting Download report "before all" hook for "user can access download link"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
alerting 234 235 +1
cases 807 808 +1
observability 1045 1046 +1
securitySolution 5654 5655 +1
triggersActionsUi 790 791 +1
total +5

Async chunks

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

id before after diff
alerting 92.4KB 92.7KB +271.0B
cases 480.1KB 480.4KB +269.0B
observability 460.1KB 460.3KB +269.0B
securitySolution 17.9MB 17.9MB +279.0B
triggersActionsUi 1.6MB 1.6MB +92.0B
total +1.2KB

Page load bundle

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

id before after diff
triggersActionsUi 121.7KB 122.0KB +279.0B

History

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

@cnasikas cnasikas self-requested a review September 3, 2024 08:54
Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Verified locally for stack and observability rules. Works as expected. 👍

appMockRender = createAppMockRenderer();
(getIsExperimentalFeatureEnabled as jest.Mock).mockImplementation(() => false);
afterEach(() => {
appMockRender.queryClient.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 😄

'.alerts-observability.uptime.alerts-*',
'.alerts-observability.metrics.alerts-*',
'.alerts-observability.logs.alerts-*',
'.alerts-observability.apm.alerts-*',
]);
],
hasReadIndexPrivilege: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to use this field at the end, could you please add some tests for hasReadIndexPrivilege: true behaviour?

Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

obs-ux-management code change lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ResponseOps][AlertsTable] Permission errors with filters and controls in the stack alerts table
6 participants