Skip to content

Refactor building role from single role descriptor #91107

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.

Sign up for GitHub

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

Merged
n1v0lg merged 11 commits into from
Oct 26, 2022
Merged

Refactor building role from single role descriptor #91107

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
61c20c8
Refactor building role from role descriptor
n1v0lg Oct 24, 2022
7417d3d
Tweaks
n1v0lg Oct 24, 2022
d22e0df
Remove cache nullability
n1v0lg Oct 24, 2022
7408109
Checkstyle
n1v0lg Oct 24, 2022
7e57b5f
Clean up
n1v0lg Oct 24, 2022
15898c4
One more
n1v0lg Oct 24, 2022
bf37d35
Merge branch 'main' into refactor-role-from-role-descriptor
n1v0lg Oct 25, 2022
20af8a5
Null check
n1v0lg Oct 25, 2022
f7679f4
Merge branch 'main' into refactor-role-from-role-descriptor
n1v0lg Oct 26, 2022
e9e83ab
Add assertion
n1v0lg Oct 26, 2022
f91e5c2
Revert "Add assertion"
n1v0lg Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 49 additions & 59 deletions ...lugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Predicate;

Expand Down Expand Up @@ -177,10 +178,6 @@ static Builder builder(RestrictedIndices restrictedIndices, String... names) {
return new Builder(restrictedIndices, names);
}

static Builder builder(RoleDescriptor rd, FieldPermissionsCache fieldPermissionsCache, RestrictedIndices restrictedIndices) {
return new Builder(rd, fieldPermissionsCache, restrictedIndices);
}

class Builder {

private final String[] names;
Expand All @@ -196,26 +193,6 @@ private Builder(RestrictedIndices restrictedIndices, String[] names) {
this.names = names;
}

private Builder(RoleDescriptor rd, @Nullable FieldPermissionsCache fieldPermissionsCache, RestrictedIndices restrictedIndices) {
// 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 == rd.hasRemoteIndicesPrivileges();
this.names = new String[] { rd.getName() };
cluster(Sets.newHashSet(rd.getClusterPrivileges()), Arrays.asList(rd.getConditionalClusterPrivileges()));
groups.addAll(convertFromIndicesPrivileges(rd.getIndicesPrivileges(), fieldPermissionsCache));

final RoleDescriptor.ApplicationResourcePrivileges[] applicationPrivileges = rd.getApplicationPrivileges();
for (RoleDescriptor.ApplicationResourcePrivileges applicationPrivilege : applicationPrivileges) {
applicationPrivs.add(convertApplicationPrivilege(applicationPrivilege));
}

String[] rdRunAs = rd.getRunAs();
if (rdRunAs != null && rdRunAs.length > 0) {
this.runAs(new Privilege(Sets.newHashSet(rdRunAs), rdRunAs));
}
this.restrictedIndices = restrictedIndices;
}

public Builder cluster(Set<String> privilegeNames, Iterable<ConfigurableClusterPrivilege> configurableClusterPrivileges) {
ClusterPermission.Builder builder = ClusterPermission.builder();
if (privilegeNames.isEmpty() == false) {
Expand Down Expand Up @@ -314,41 +291,6 @@ public SimpleRole build() {
return new SimpleRole(names, cluster, indices, applicationPermission, runAs, remoteIndices);
}

static List<IndicesPermissionGroupDefinition> convertFromIndicesPrivileges(
RoleDescriptor.IndicesPrivileges[] indicesPrivileges,
@Nullable FieldPermissionsCache fieldPermissionsCache
) {
List<IndicesPermissionGroupDefinition> list = new ArrayList<>(indicesPrivileges.length);
for (RoleDescriptor.IndicesPrivileges privilege : indicesPrivileges) {
final FieldPermissions fieldPermissions;
if (fieldPermissionsCache != null) {
fieldPermissions = fieldPermissionsCache.getFieldPermissions(privilege.getGrantedFields(), privilege.getDeniedFields());
} else {
fieldPermissions = new FieldPermissions(
new FieldPermissionsDefinition(privilege.getGrantedFields(), privilege.getDeniedFields())
);
}
final Set<BytesReference> query = privilege.getQuery() == null ? null : Collections.singleton(privilege.getQuery());
list.add(
new IndicesPermissionGroupDefinition(
IndexPrivilege.get(Sets.newHashSet(privilege.getPrivileges())),
fieldPermissions,
query,
privilege.allowRestrictedIndices(),
privilege.getIndices()
)
);
}
return list;
}

static Tuple<ApplicationPrivilege, Set<String>> convertApplicationPrivilege(RoleDescriptor.ApplicationResourcePrivileges arp) {
return new Tuple<>(
new ApplicationPrivilege(arp.getApplication(), Sets.newHashSet(arp.getPrivileges()), arp.getPrivileges()),
Sets.newHashSet(arp.getResources())
);
}

private static class IndicesPermissionGroupDefinition {
private final IndexPrivilege privilege;
private final FieldPermissions fieldPermissions;
Expand All @@ -371,4 +313,52 @@ private IndicesPermissionGroupDefinition(
}
}
}

static SimpleRole buildFromRoleDescriptor(
final RoleDescriptor roleDescriptor,
final FieldPermissionsCache fieldPermissionsCache,
final RestrictedIndices restrictedIndices
) {
// 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);
Copy link
Contributor Author

@n1v0lg n1v0lg Oct 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.


final Builder builder = builder(restrictedIndices, roleDescriptor.getName());

builder.cluster(
Sets.newHashSet(roleDescriptor.getClusterPrivileges()),
Arrays.asList(roleDescriptor.getConditionalClusterPrivileges())
);

for (RoleDescriptor.IndicesPrivileges indexPrivilege : roleDescriptor.getIndicesPrivileges()) {
builder.add(
fieldPermissionsCache.getFieldPermissions(
new FieldPermissionsDefinition(indexPrivilege.getGrantedFields(), indexPrivilege.getDeniedFields())
),
indexPrivilege.getQuery() == null ? null : Collections.singleton(indexPrivilege.getQuery()),
IndexPrivilege.get(Sets.newHashSet(indexPrivilege.getPrivileges())),
indexPrivilege.allowRestrictedIndices(),
indexPrivilege.getIndices()
);
}

for (RoleDescriptor.ApplicationResourcePrivileges applicationPrivilege : roleDescriptor.getApplicationPrivileges()) {
builder.addApplicationPrivilege(
new ApplicationPrivilege(
applicationPrivilege.getApplication(),
Sets.newHashSet(applicationPrivilege.getPrivileges()),
applicationPrivilege.getPrivileges()
),
Sets.newHashSet(applicationPrivilege.getResources())
);
}
Comment on lines +346 to +355
Copy link
Member

@ywangd ywangd Oct 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@n1v0lg n1v0lg Oct 26, 2022

Choose a reason for hiding this comment

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

Makes sense. Added the assertion 👍

Copy link
Contributor Author

@n1v0lg n1v0lg Oct 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ywangd ywangd Oct 26, 2022

Choose a reason for hiding this comment

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

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!


final String[] rdRunAs = roleDescriptor.getRunAs();
if (rdRunAs != null && rdRunAs.length > 0) {
builder.runAs(new Privilege(Sets.newHashSet(rdRunAs), rdRunAs));
}

return builder.build();
}
}
6 changes: 3 additions & 3 deletions ...src/test/java/org/elasticsearch/xpack/core/security/authz/permission/SimpleRoleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ public void testEmptyRoleHasNoEmptyListOfNames() {
}

public void testHasPrivilegesCache() throws ExecutionException {
final SimpleRole role = Role.builder(
final SimpleRole role = Role.buildFromRoleDescriptor(
new RoleDescriptor(randomAlphaOfLengthBetween(3, 8), new String[] { "monitor" }, null, null),
null,
new FieldPermissionsCache(Settings.EMPTY),
RESTRICTED_INDICES
).build();
);

// cache is null to begin with
assertThat(role.getHasPrivilegesCache(), nullValue());
Expand Down
Loading