update permissions according to backend #1480
Conversation
Signed-off-by: Derek Ho <dxho@amazon.com>
derek-ho
requested review from
cliu123,
cwperks,
DarshitChanpura,
davidlago,
peternied,
RyanL1997 and
stephen-crawford
as code owners
|
@derek-ho Can you include a link to the relevant backend code in the PR description? |
| 'indices:data/write/bulk', | ||
| 'indices:data/write/bulk*', |
There was a problem hiding this comment.
backend says equal to indices:data/write/bulk - should it include the * as well?
| 'indices:data/read/mget', | ||
| 'indices:data/read/mget*', |
There was a problem hiding this comment.
backend says equal to indices:data/read/mget should it include the * as well?
| 'indices:data/read/msearch', | ||
| 'indices:data/read/msearch/template', |
There was a problem hiding this comment.
backend says equal to indices:data/read/msearch - should it include the /template as well
| 'indices:data/read/mtv', | ||
| 'indices:data/read/mtv*', |
There was a problem hiding this comment.
backend says equal to indices:data/read/mtv should it include the * as well
Several areas where not sure if I should move things back on the front end or backend should be changed to starts with instead of strict equals |
Codecov Report
@@ Coverage Diff @@
## main #1480 +/- ##
=======================================
Coverage 65.62% 65.62%
=======================================
Files 93 93
Lines 2307 2307
Branches 309 309
=======================================
Hits 1514 1514
Misses 725 725
Partials 68 68
|
|
It helps to see these written out and categorized correctly on the frontend. Thank you @derek-ho! I found it interesting that scroll was on the list so I dove a little into how security for scroll works. Since scroll is associated with indices, at some point it needs to be determined if a user has permission to query the underlying indices and as a cluster permission index patterns are not considered. Under the hood, Scroll calls on this transport action in core: That then calls on SearchScrollQueryAndFetchAsyncAction Which calls here: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/action/search/SearchTransportService.java#L285-L298 and that executes the transport action indices:data/read/search[phase/query+fetch/scroll]. Since the transport action |
There was a problem hiding this comment.
@derek-ho Thanks for looking into this space - while based on that filter from the PrivilegesEvaluator these changes look mostly inline - what is broken without this change, is there an issue with more details?
Since permissions are created and stored in the security index which we do not update on upgrade, is a migrate needed to move these cluster permissions from the list of index permissions?
My understanding is it's just a minor UI categorization bug/doesn't require any migrations of any kind. Created an issue #1481 to detail further. Maybe @RyanL1997 can confirm whether my understanding is correct |
|
After the change: 
|
|
@peternied This change updates the categorization as @derek-ho pointed out, but will also be used (in a subsequent PR) to populate the Cluster Permission and Index Permission dropdowns with the correct list of permissions. Right now, all permissions show on both dropdowns and it is quite confusing. Each dropdown should only contain the list of permissions pertinent to that dropdown. |
There was a problem hiding this comment.
I've created an issue for checking on if a migration is needed or not, but that shouldn't prevent this change from fixing the UX that is broken - thanks for the contribution @derek-ho !
Signed-off-by: Derek Ho <dxho@amazon.com> (cherry picked from commit 8dab6a3)
Signed-off-by: Derek Ho <dxho@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>
Description
Moves some permissions around according to:
https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java#L664-L676
Category
[Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation]
Why these changes are required?
What is the old behavior before changes and new behavior after changes?
Issues Resolved
[List any issues this PR will resolve (Is this a backport? If so, please add backport PR # and/or commits #)]
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
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.