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

Handle other permission group types #1715

Merged

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Jan 3, 2024

Description

Slightly updates the logic for calculating Other permission groups in the permissions dropdowns to ensure that groups with type not belonging to ['cluster', 'index', 'kibana'] are accounted for. The linked PR handles the case for custom action groups where type is never assigned, but doesn't account for the static action group unlimited that belongs to the "all" type. This is the only pre-defined action group that does not belong to cluster, index or kibana type.

Category

Bug fix

Issues Resolved

Related PR and issue:

Testing

Screenshot 2024-01-03 at 11 16 05 AM

Check List

  • New functionality includes testing
  • New functionality has been documented
  • 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.

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (09b2f59) 67.09% compared to head (354b2e0) 67.09%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1715   +/-   ##
=======================================
  Coverage   67.09%   67.09%           
=======================================
  Files          94       94           
  Lines        2404     2404           
  Branches      318      318           
=======================================
  Hits         1613     1613           
  Misses        713      713           
  Partials       78       78           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for this change.

The underlying bug is pretty bad worse as action groups are customizable. In the distribution we have a set that we include by default, but they can be customized by the cluster operators to whatever they'd like. Unknowingly action groups might have been created that were hidden from the UX.

@cwperks cwperks added the v2.12.0 Items targeting 2.12.0 label Jan 3, 2024
Copy link
Collaborator

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

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

Ah I missed this - I thought they would just use * for all access?

@cwperks
Copy link
Member Author

cwperks commented Jan 4, 2024

@derek-ho I just checked on the UI and its not possible to assign cluster or index permissions unless they appear in the dropdown so its not possible to assign * from the UI directly.

I wonder if it should allow wildcard matches as long as the value entered matches at least 1 action.

@DarshitChanpura DarshitChanpura merged commit ac38d9d into opensearch-project:main Jan 4, 2024
12 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 4, 2024
Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit ac38d9d)
peternied pushed a commit that referenced this pull request Jan 4, 2024
Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit ac38d9d)

Co-authored-by: Craig Perkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch v2.12.0 Items targeting 2.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants