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

[docs] Document new role description field #108422

Merged

Conversation

slobodanadamovic
Copy link
Contributor

@slobodanadamovic slobodanadamovic commented May 8, 2024

Update API docs to include new description field (introduced in #107088) and add descriptions for all built-in roles.

@slobodanadamovic slobodanadamovic added >docs General docs changes :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team labels May 8, 2024
@slobodanadamovic slobodanadamovic self-assigned this May 8, 2024
Copy link
Contributor

github-actions bot commented May 8, 2024

Documentation preview:

+ "as well as updating user profile data for the kibana-* namespace. "
+ "Additionally, this role grants read access to the .monitoring-* indices "
+ "and read and write access to the .reporting-* indices. "
+ "Note: This role should not be assigned to users as the granted permissions may change between releases."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a verbatim copy of kibana_system role's description. The documentation seems to be outdated, since the permissions have changed quite a lot since the documentation was first written.
Not sure if we should cover them all. But I do think it's worth revisiting description to be more explicit in stating that this its intention is for system use by Kibana and that it should not be granted to regular users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good catch! I can take this back to my team to discuss updates, but I think this is fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me.

* would be inconsistent and require handling backwards compatibility.
* Hence why we have to remove them before create/update of API key roles.
*/
static Set<RoleDescriptor> removeUserRoleDescriptorDescriptions(Set<RoleDescriptor> userRoleDescriptors) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made package protected for testing.

@slobodanadamovic slobodanadamovic marked this pull request as ready for review May 10, 2024 16:26
@slobodanadamovic slobodanadamovic requested review from a team as code owners May 10, 2024 16:26
@elasticsearchmachine elasticsearchmachine added the Team:Docs Meta label for docs team label May 10, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@kc13greiner kc13greiner self-requested a review May 10, 2024 16:43
Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

LGTM!

+ "as well as updating user profile data for the kibana-* namespace. "
+ "Additionally, this role grants read access to the .monitoring-* indices "
+ "and read and write access to the .reporting-* indices. "
+ "Note: This role should not be assigned to users as the granted permissions may change between releases."
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good catch! I can take this back to my team to discuss updates, but I think this is fine for now.

Copy link
Contributor

@shainaraskas shainaraskas left a comment

Choose a reason for hiding this comment

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

docs lgtm with a couple of small comments

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM

@slobodanadamovic slobodanadamovic merged commit 77ce605 into elastic:main May 14, 2024
20 checks passed
slobodanadamovic added a commit that referenced this pull request May 16, 2024
This PR adds missing role description for the `transport_client role`, 
and a test to enforce that all reserved roles are described. 
The description also serves as self-documentation for roles, 
thus it is reasonable to make this a requirement for all reserved roles.

Relates to #108422, which included descriptions for other reserved roles.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Docs Meta label for docs team Team:Security Meta label for security team v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants