Refactor building role from single role descriptor#91107
Refactor building role from single role descriptor#91107n1v0lg merged 11 commits intoelastic:mainfrom
Conversation
n1v0lg
added
>refactoring
:Security/Security
n1v0lg
changed the title
| // TODO handle this when we introduce remote index privileges for built-in users and roles. That's the only production code | ||
| // using this builder | ||
| assert false == roleDescriptor.hasRemoteIndicesPrivileges(); | ||
| Objects.requireNonNull(fieldPermissionsCache); |
There was a problem hiding this comment.
fieldPermissionsCache was nulleable before, however it was only ever null in test code. Opting to make it not nulleable since test code should not impact production code. Passing an instantiated FieldPermissionsCache in the relevant tests does not incur a notable overhead.
...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java
Show resolved
Hide resolved
n1v0lg
changed the title
|
Pinging @elastic/es-security (Team:Security) |
| for (RoleDescriptor.ApplicationResourcePrivileges applicationPrivilege : roleDescriptor.getApplicationPrivileges()) { | ||
| builder.addApplicationPrivilege( | ||
| new ApplicationPrivilege( | ||
| applicationPrivilege.getApplication(), | ||
| Sets.newHashSet(applicationPrivilege.getPrivileges()), | ||
| applicationPrivilege.getPrivileges() | ||
| ), | ||
| Sets.newHashSet(applicationPrivilege.getResources()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
It's not part of the current code. But I think it is worthwhile to assert that all privilege names are patterns (e.g. *) instead of concrete names (this is already the case for all production code and likely tests as well). The code here does not really work if there are concrete names because we need to look up what actions the concrete name include from the security index, e.g. kibana's space_all includes login:, saved_object:tag/get and a bunch of other actions.
Concretely, what I am suggesting is something like
assert Stream.of(applicationPrivilege.getPrivileges()).noneMatch(ApplicationPrivilege::isValidPrivilegeName)
similar to the check in NativePrivilegeStore.
There was a problem hiding this comment.
Makes sense. Added the assertion 👍
There was a problem hiding this comment.
There's quite a few tests that fail this assertion, e.g., testElasticFleetServerPrivileges because we build a role for fleet account which includes concrete app privilge names. I'll merge without the assertion and follow up with a PR that addresses this, to better separate pure refactoring from bigger "functional" changes.
There was a problem hiding this comment.
I was wondering whether it would catch something like this. Yeah, separate PR is fine. Should just be a test issue, but it might need some work to get around it. Thanks!
Building a role from a single role descriptor previously only worked correctly for application privileges with wildcard patterns. This PR adds support for concrete name look up and ports the relevant tests. Relates: #91107 (comment)
3 participants
This PR refactors functionality for building a role from a single role
descriptor. This used to be handled by a specialized constructor of the
Role.Builder class that accessed internal fields. This PR consolidates
this logic into a static method that uses the builder's canonical
methods instead. The method is used to construct the roles of internal
users, from static role descriptors, as well as in test code.
Relates: #90614 (comment)