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

Rbac support #882

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

Conversation

smuthukaruppannp
Copy link

Description

Supports advanced backend role security f

Issues Resolved

#860

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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
Copy link
Author

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

@plin13
Copy link

plin13 commented Feb 20, 2024

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

@AWSHurneyt
Copy link
Collaborator

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

@smuthukaruppannp
Copy link
Author

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

@AWSHurneyt
Copy link
Collaborator

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

name="roles"
formRow
rowProps={{
label: 'Backend roles',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed the UI element with our UI developer. Could you change the label to the following, and remove the helpText?

label: (
            <div>
              <EuiText size={'xs'}>
                <strong>Backend roles</strong>
                <i> - optional </i>
              </EuiText>
              <EuiText color={'subdued'} size={'xs'} style={{ width: '400px' }}>
                Specify role-based access control (RBAC) backend roles.{' '}
                <EuiLink external href={'https://opensearch.org/docs/latest/observing-your-data/alerting/security/'} target={'_blank'}>
                  Learn more
                </EuiLink>
              </EuiText>
            </div>
          ),

@smuthukaruppannp
Copy link
Author

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

Signed-off-by: Seth Muthukaruppan <seth.muthukaruppan@netapp.com>
Signed-off-by: Seth Muthukaruppan <seth.muthukaruppan@netapp.com>
Signed-off-by: Seth Muthukaruppan <seth.muthukaruppan@netapp.com>
Signed-off-by: Seth Muthukaruppan <seth.muthukaruppan@netapp.com>
@scubbx
Copy link

scubbx commented Sep 12, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Planned work items
Development

Successfully merging this pull request may close these issues.

4 participants