-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Fix privileges for predefined roles #72061
Conversation
In elastic#71904 we added two new predefined roles, editor and viewer in elasticsearch. This change adds a missing application privilege to these roles. This is only needed in 7.x as the behavior has changed in Kibana, starting 8.0 and this extra privilege is not necessary.
Pinging @elastic/es-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.
I left a couple of changes that I think are necessary to make it work properly.
.privileges("read").build() }, | ||
.privileges("read").build(), | ||
RoleDescriptor.ApplicationResourcePrivileges.builder() | ||
.application("kibana-.*") |
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.
.application("kibana-.*") | |
.application("kibana-*") |
I think this is the right syntax, given the comment in #71710 (comment) and a chat I had with @legrego
I'll leave to Larry to comment futher.
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.
You're probably right, let's wait for @legrego to verify
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.
@jkakavas, @bytebilly's suggestion is the correct syntax: There should not be a .
in the application name for "reserved" privileges
.privileges("all").build() }, | ||
.privileges("all").build(), | ||
RoleDescriptor.ApplicationResourcePrivileges.builder() | ||
.application("kibana-.*") |
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.
.application("kibana-.*") | |
.application("kibana-*") |
I think this is the right syntax, given the comment in #71710 (comment) and a chat I had with @legrego
I'll leave to Larry to comment futher.
@elasticmachine update branch |
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.
LGTM 👍
In elastic#71904 we added two new predefined roles, editor and viewer in elasticsearch. This change adds a missing application privilege to these roles. This is only needed in 7.x as the behavior has changed in Kibana, starting 8.0 and this extra privilege is not necessary.
In #71904 we added two new predefined roles, editor and viewer in elasticsearch. This change adds a missing application privilege to these roles. This is only needed in 7.x as the behavior has changed in Kibana, starting 8.0 and this extra privilege is not necessary.
In #71904 we added two new predefined roles, editor and viewer in
elasticsearch. This change adds a missing application privilege to
these roles. This is only needed in 7.x as the behavior has changed
in Kibana, starting 8.0 and this extra privilege is not necessary.
Marking as non-issue as this is not yet released.