alerting-dashboards-plugin icon indicating copy to clipboard operation
alerting-dashboards-plugin copied to clipboard

Rbac support

Open smuthukaruppannp opened this issue 1 year ago • 7 comments

Description

Supports advanced backend role security f

Issues Resolved

#860

Check List

  • [x] New functionality includes testing.
    • [x] All tests pass
  • [ ] New functionality has been documented.
    • [ ] New functionality has javadoc added
  • [x] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

smuthukaruppannp avatar Feb 15 '24 06:02 smuthukaruppannp

@AWSHurneyt this PR adds the ability to assign backend roles in the UI as per our discussion. Please review and provide your feedback

smuthukaruppannp avatar Feb 15 '24 15:02 smuthukaruppannp

@AWSHurneyt @brijos @dtaivpp Hi following up on this PR. It'll be an incredibly useful addition that we'd like to include , hopefully backported in 2.12 / 2.13 release.

plin13 avatar Feb 20 '24 15:02 plin13

@smuthukaruppannp @plin13 The cut off for changes for 2.12 was Feb. 6th unfortunately, but we'll get this reviewed for 2.13.

AWSHurneyt avatar Feb 20 '24 17:02 AWSHurneyt

@AWSHurneyt, a gentle reminder to review this PR when you get a chance. Thanks.

smuthukaruppannp avatar Mar 19 '24 14:03 smuthukaruppannp

@smuthukaruppannp I was actually just discussing this PR with the team last week.

I showed your changes to one of our UI developers to confirm that the help text, and whatnot looked appropriate. He recommended a few, minor changes. I'll add a comment in the code diff of the PR.

While testing your changes last week, I noticed that the role selection dropdown would clear all selections when editing a monitor. Did you experience that as well?

I did some troubleshooting, and came across this PR from a few years ago (https://github.com/opensearch-project/alerting/pull/201). It appears that we needed to remove all user/role information from the output of the SearchMonitor, and GetMonitor API calls as the security team determined we should not be returning any info like that in those API responses. This means that the frontend isn't able to retrieve that list of roles when editing a monitor.

We'll need to do a more extensive review with the security team before we can merge in this PR; not because of your changes, but because of changes to the backend API that will be needed to support your changes. Because we already had to remove the user/role info from the responses, we will likely have to develop a new way to retrieve that info. This means we'll miss the v2.13 release for these changes, as API changes would require approval, and testing by the security team.

AWSHurneyt avatar Mar 19 '24 15:03 AWSHurneyt

@smuthukaruppannp I was actually just discussing this PR with the team last week.

I showed your changes to one of our UI developers to confirm that the help text, and whatnot looked appropriate. He recommended a few, minor changes. I'll add a comment in the code diff of the PR.

While testing your changes last week, I noticed that the role selection dropdown would clear all selections when editing a monitor. Did you experience that as well?

I did some troubleshooting, and came across this PR from a few years ago (opensearch-project/alerting#201). It appears that we needed to remove all user/role information from the output of the SearchMonitor, and GetMonitor API calls as the security team determined we should not be returning any info like that in those API responses. This means that the frontend isn't able to retrieve that list of roles when editing a monitor.

We'll need to do a more extensive review with the security team before we can merge in this PR; not because of your changes, but because of changes to the backend API that will be needed to support your changes. Because we already had to remove the user/role info from the responses, we will likely have to develop a new way to retrieve that info. This means we'll miss the v2.13 release for these changes, as API changes would require approval, and testing by the security team.

Thanks for the feedback @AWSHurneyt. I will make the UI element changes.

During monitor edit, we cannot display the backend roles as current GET monitor API does not return that information. I have created feature request 1429 for it

To populate the backend role dropdown in the UI, i am using already available and documented security APIs _plugins/_security/api/account and _plugins/_security/api/rolesmapping. Users will be pulling their own account information and system information only if they have the right security permissions. These APIs can be used outside of the alerting UI to get the exact same information. So I am not sure how this can cause a security issue. Please advise.

smuthukaruppannp avatar Mar 25 '24 16:03 smuthukaruppannp

Hello! What is the status of this pull request? What needs to be done for this to be merged? I am actually in need of this exact functionality and am excited to have found that it already has been implemented.

scubbx avatar Sep 12 '24 16:09 scubbx