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

Moving cluster indices permission to cluster section #1656 #7161

Conversation

AntonEliatra
Copy link
Contributor

@AntonEliatra AntonEliatra commented May 15, 2024

Description

Moving cluster wide Index permissions to cluster permission section

Issues Resolved

Closes #2359
Closes #1656
Closes #621

Checklist

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

@AntonEliatra
Copy link
Contributor Author

Converted to draft, as found permissions in Dashboards UI that user can only add as cluster, but should be added as index permission,

indices:data/write/bulk*

@hdhalter hdhalter added security backport 2.14 PR: Backport label for 2.14 2 - In progress Issue/PR: The issue or PR is in progress. labels May 15, 2024
@AntonEliatra AntonEliatra marked this pull request as ready for review May 21, 2024 16:17
@hdhalter hdhalter added 3 - Tech review PR: Tech review in progress and removed 2 - In progress Issue/PR: The issue or PR is in progress. labels May 22, 2024
@AntonEliatra
Copy link
Contributor Author

While working on this PR I came across incorrect/outdated API documentation and raised issue to fix it:
#7234

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Looks good thanks Anton!

@hdhalter hdhalter added 4 - Doc review PR: Doc review in progress and removed 3 - Tech review PR: Tech review in progress labels May 24, 2024
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
Co-authored-by: Naarcha-AWS <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>
@hdhalter
Copy link
Contributor

@Naarcha-AWS , if this looks good, it can go to Nathan. An editorial request has already been submitted.

@Naarcha-AWS Naarcha-AWS added 5 - Editorial review PR: Editorial review in progress and removed 4 - Doc review PR: Doc review in progress labels Jun 4, 2024
@Naarcha-AWS
Copy link
Collaborator

@natebower: This is ready for editorial review.

Copy link
Collaborator

@natebower natebower left a comment

Choose a reason for hiding this comment

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

@AntonEliatra @Naarcha-AWS Please see my changes and confirm the phrasing I've flagged on lines 193 and 514. Thanks!

_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
| `indices:data/read/scroll` | Permission to scroll data. |
| `indices:data/read/scroll/clear` | Permission to clear read scroll data. |
| `indices:data/read/scroll` | Permission to scroll data. This setting needs to be configured as both a cluster and index level permission. |
| `indices:data/read/scroll/clear` | Permission to clear read scroll data. This setting needs to be configured as both a cluster and index level permission. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is "read scroll data" a thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @natebower I'm not sure I follow, You mean read/scroll to scroll through data/results or read/scroll/data - which I haven't come across

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AntonEliatra I suspect that "read scroll data" is meant to be past tense, but it reads as though "read scroll data" is a noun. Can we rephrase for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see what you mean, will fix now

_security/access-control/permissions.md Outdated Show resolved Hide resolved
_security/access-control/permissions.md Outdated Show resolved Hide resolved
Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>
Copy link
Collaborator

@natebower natebower left a comment

Choose a reason for hiding this comment

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

Please rephrase lines 193 and 514. Thanks!

Signed-off-by: AntonEliatra <[email protected]>
Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>
Copy link
Collaborator

@natebower natebower left a comment

Choose a reason for hiding this comment

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

Thanks @AntonEliatra! LGTM

@Naarcha-AWS Naarcha-AWS merged commit 88bab65 into opensearch-project:main Jun 5, 2024
5 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 5, 2024
* Moving cluster indices permission to cluster section #1656

Signed-off-by: AntonEliatra <[email protected]>

* updating index and cluster permissions

Signed-off-by: AntonEliatra <[email protected]>

* removing empty space

Signed-off-by: AntonEliatra <[email protected]>

* fixing vale errors

Signed-off-by: AntonEliatra <[email protected]>

* adding more permissions to the list

Signed-off-by: AntonEliatra <[email protected]>

* Apply suggestions from code review

Co-authored-by: Naarcha-AWS <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>

* Update permissions.md

Signed-off-by: AntonEliatra <[email protected]>

* Update _security/access-control/permissions.md

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>

---------

Signed-off-by: AntonEliatra <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>
Co-authored-by: Naarcha-AWS <[email protected]>
Co-authored-by: Nathan Bower <[email protected]>
(cherry picked from commit 88bab65)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Editorial review PR: Editorial review in progress backport 2.14 PR: Backport label for 2.14 security
Projects
None yet
5 participants