-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Roles] Added transformation of application * privilege to all #181400
[Roles] Added transformation of application * privilege to all #181400
Conversation
/ci |
Pinging @elastic/kibana-security (Team:Security) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, Elena!
I just found one issue. If I create a role via ES API with wildcard application privileges (like superuser
and system_indices_superuser
both have), and then load and save the role in the UI (making no changes), the role ends up with an additional entry that correlates to the translated 'all' privilege.
"applications": [ { "application": "*", "privileges": [ "*" ], "resources": [ "*" ] } ],
becomes
"applications": [ { "application": "kibana-.kibana", "privileges": [ "all" ], "resources": [ "*" ] }, { "application": "*", "privileges": [ "*" ], "resources": [ "*" ] } ],
And it also shows up in the UI:
It will also occur if a user edits the translated privileges in the UI. When the privilege is updated, it will retain the original wildcard as well as a new 'kibana':'all'.
We may want to intercept this on save/update and have a special case for when a role already contains the wild card so it is either retained (if assigning all) or overwritten (if changed), and potentially display a message to the user when a role contains the superuser style wildcard.
cc: @legrego any thoughts?
Great find, @jeramysoucy. Thanks for your attention to detail here.
I'm not opposed to exploring some sort of interception if it's not an unreasonable amount of effort, and not too hacky. Otherwise, we should consider alternate approaches. |
@legrego @jeramysoucy privilege with The alternative can be if we return the application privilege with strongest weight in that case? Because in the end the strongest privileges will be applied. WDYT?
|
@jeramysoucy Thanks for bringing it in. Looked carefully at the setup, it seems that my normalization of application wildcard privilege on the server brought more problems than it eventually solved. Discussed it with @legrego, we've decided to not normalize it to |
@elena-shostak Sounds good. Do we want to disallow the delete action as well? |
I think we can, to make it consistent with superuser role display. |
3225b5b
to
5feadd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry in advance. I found a couple of things.
- Can we notify the user somehow that a base wildcard privilege is special? The action icons are removed, but there is nothing else to let the user know what's going on. Maybe an icon tip where the action icons usually are? cc: @legrego do we need to check with the design team or doc team for approach/wording?
- If I view a role with the base wildcard and click Update, there is an error:
This is tricky, because if we disable the update button then no changes can be made to ES privileges for the role. So I think we have to trap the Kibana base wildcard on update, and only apply any ES changes. - If I view a role with the base wildcard, I still have options to add a Kibana privilege and view a privilege summary.
The summary shows all "none" (expected since this is a special case), but maybe we should remove the ability to view this is the base wildcard is the only privilege?
I think both of these options,Add Kibana privilege
andView privilege summary
, should be removed for roles with the base wildcard.
...les/edit_role/privileges/kibana/space_aware_privilege_section/privilege_space_table.test.tsx
Show resolved
Hide resolved
I'm torn on this. On one hand, it makes sense to give knowledgeable administrators this information, but this could lead to more confusion for the majority of our administrators. To my knowledge, Kibana is the primary consumer of application privileges. There might be an enterprise search use case, but they have their own bespoke authorization model that's never been compatible with our role management UI/API. Telling them this grants access to additional application privileges in effect doesn't say much, because it's really just Kibana in practice.
I wonder if adding API support for other application privileges would be a helpful prerequisite? #21503
++ |
...ent/roles/edit_role/privileges/kibana/privilege_form_calculator/privilege_form_calculator.ts
Show resolved
Hide resolved
Changes addressed:
Let me know if that works and makes sense. |
1d2f18f
to
2a1484a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had one suggestion in the new test, to check for a read-only button render. I am approving now so that i don't hold up this PR any further.
I think making roles with the app wildcard privilege read-only in the UI is fine. These roles could only have been create/updated via the ES API anyway. I'm not sure how I feel about there not being any message to clarify this to the end user, but this is something we can come back to if needed.
Great work, Elena!
x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.test.tsx
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…181165) ## Summary Added endpoint `GET kbn:/internal/security/roles/{space-id}` to get all roles for provided space id. **Note**: changes needed for application `*` privileges were cherry-picked [to a separate PR].(#181400) ## Example Request `GET kbn:/internal/security/roles/space-b` Response ``` [ { "name": "role-a", "metadata": {}, "transient_metadata": { "enabled": true }, "elasticsearch": { "cluster": [ "all" ], "indices": [], "run_as": [] }, "kibana": [ { "base": [], "feature": { "dev_tools": [ "all" ] }, "spaces": [ "default", "space-b" ] } ], "_transform_error": [], "_unrecognized_applications": [] }, { "name": "superuser", "metadata": { "_reserved": true }, "transient_metadata": {}, "elasticsearch": { "cluster": [ "all" ], "indices": [ { "names": [ "*" ], "privileges": [ "all" ], "allow_restricted_indices": false }, { "names": [ "*" ], "privileges": [ "monitor", "read", "view_index_metadata", "read_cross_cluster" ], "allow_restricted_indices": true } ], "remote_indices": [ { "names": [ "*" ], "privileges": [ "all" ], "allow_restricted_indices": false, "clusters": [ "*" ] }, { "names": [ "*" ], "privileges": [ "monitor", "read", "view_index_metadata", "read_cross_cluster" ], "allow_restricted_indices": true, "clusters": [ "*" ] } ], "run_as": [ "*" ] }, "kibana": [ { "base": [ "all" ], "feature": {}, "spaces": [ "*" ] } ], "_transform_error": [], "_unrecognized_applications": [ "*" ] } ] ``` ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) __Fixes: https://github.com/elastic/kibana/issues/180718__ --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
…lastic#181165) ## Summary Added endpoint `GET kbn:/internal/security/roles/{space-id}` to get all roles for provided space id. **Note**: changes needed for application `*` privileges were cherry-picked [to a separate PR].(elastic#181400) ## Example Request `GET kbn:/internal/security/roles/space-b` Response ``` [ { "name": "role-a", "metadata": {}, "transient_metadata": { "enabled": true }, "elasticsearch": { "cluster": [ "all" ], "indices": [], "run_as": [] }, "kibana": [ { "base": [], "feature": { "dev_tools": [ "all" ] }, "spaces": [ "default", "space-b" ] } ], "_transform_error": [], "_unrecognized_applications": [] }, { "name": "superuser", "metadata": { "_reserved": true }, "transient_metadata": {}, "elasticsearch": { "cluster": [ "all" ], "indices": [ { "names": [ "*" ], "privileges": [ "all" ], "allow_restricted_indices": false }, { "names": [ "*" ], "privileges": [ "monitor", "read", "view_index_metadata", "read_cross_cluster" ], "allow_restricted_indices": true } ], "remote_indices": [ { "names": [ "*" ], "privileges": [ "all" ], "allow_restricted_indices": false, "clusters": [ "*" ] }, { "names": [ "*" ], "privileges": [ "monitor", "read", "view_index_metadata", "read_cross_cluster" ], "allow_restricted_indices": true, "clusters": [ "*" ] } ], "run_as": [ "*" ] }, "kibana": [ { "base": [ "all" ], "feature": {}, "spaces": [ "*" ] } ], "_transform_error": [], "_unrecognized_applications": [ "*" ] } ] ``` ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) __Fixes: https://github.com/elastic/kibana/issues/180718__ --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…ic#181400) ## Summary Added transformation of application wildcard `*` privilege to `all` to correctly filter and display roles as `superuser`. <details> <summary>[Screenshot] Kibana superuser role before change</summary> <img width="1508" alt="Screenshot 2024-04-23 at 11 20 31" src="https://github.com/elastic/kibana/assets/165678770/594cf27c-64a7-49cf-bf4b-c63adca76a55"> </details> <details> <summary>[Screenshot] Kibana superuser role after change</summary> <img width="1504" alt="Screenshot 2024-04-23 at 11 17 54" src="https://github.com/elastic/kibana/assets/165678770/96dbdaa6-f5c9-4644-82ca-f88a0f6a15b3"> </details> ### Checklist - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) __Fixes: https://github.com/elastic/kibana/issues/106561__ ## Release note Added transformation of application wildcard `*` privilege to `all` to correctly filter and display roles as `superuser`. --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Thank you all so much for this change, I'd been following an earlier issue that this PR closed and it was very happy news to see! <3 |
Summary
Added transformation of application wildcard
*
privilege toall
to correctly filter and display roles assuperuser
.[Screenshot] Kibana superuser role before change
[Screenshot] Kibana superuser role after change
Checklist
For maintainers
Fixes: #106561
Release note
Added transformation of application wildcard
*
privilege toall
to correctly filter and display roles assuperuser
.