From 5cf8b94f30037f7f4719be2bcd2bce45e513373d Mon Sep 17 00:00:00 2001 From: David Turner Date: Fri, 8 Dec 2023 12:56:05 +0000 Subject: [PATCH] Simplify CompositeRolesStore#getRoleDescriptors (#103126) This method has only one production caller, which asserts that the response always has length 1 and (if assertions are disabled) discards all but the first set of role descriptors in the response. We can push that behaviour down into the implementation simplifying things quite considerably and eliminating the allocation and copying needed to support other sizes of response. --- .../ApiKeyUserRoleDescriptorResolver.java | 10 ++---- .../authz/store/CompositeRolesStore.java | 35 ++++++------------- ...ApiKeyUserRoleDescriptorResolverTests.java | 11 +++--- .../authz/store/CompositeRolesStoreTests.java | 14 ++++---- 4 files changed, 24 insertions(+), 46 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/ApiKeyUserRoleDescriptorResolver.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/ApiKeyUserRoleDescriptorResolver.java index 17c35ecca5f13..9c54f6decb0c8 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/ApiKeyUserRoleDescriptorResolver.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/ApiKeyUserRoleDescriptorResolver.java @@ -15,7 +15,6 @@ import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; -import java.util.Collection; import java.util.Set; public class ApiKeyUserRoleDescriptorResolver { @@ -37,15 +36,10 @@ public void resolveUserRoleDescriptors(final Authentication authentication, fina return; } - rolesStore.getRoleDescriptorsList(effectiveSubject, listener.delegateFailureAndWrap(this::handleRoleDescriptorsList)); + rolesStore.getRoleDescriptors(effectiveSubject, listener.delegateFailureAndWrap(this::handleRoleDescriptors)); } - private void handleRoleDescriptorsList( - ActionListener> listener, - Collection> roleDescriptorsList - ) { - assert roleDescriptorsList.size() == 1; - final var roleDescriptors = roleDescriptorsList.iterator().next(); + private void handleRoleDescriptors(ActionListener> listener, Set roleDescriptors) { for (RoleDescriptor rd : roleDescriptors) { DLSRoleQueryValidator.validateQueryField(rd.getIndicesPrivileges(), xContentRegistry); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java index ab9ea8f772054..631a988b0f325 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java @@ -10,7 +10,6 @@ import org.apache.logging.log4j.Logger; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; -import org.elasticsearch.action.support.GroupedActionListener; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.cache.Cache; @@ -349,30 +348,18 @@ private void buildThenMaybeCacheRole( ); } - public void getRoleDescriptorsList(Subject subject, ActionListener>> listener) { - tryGetRoleDescriptorForInternalUser(subject).ifPresentOrElse( - roleDescriptor -> listener.onResponse(List.of(Set.of(roleDescriptor))), - () -> { - final List roleReferences = subject.getRoleReferenceIntersection(anonymousUser).getRoleReferences(); - final GroupedActionListener> groupedActionListener = new GroupedActionListener<>( - roleReferences.size(), - listener - ); + public void getRoleDescriptors(Subject subject, ActionListener> listener) { + tryGetRoleDescriptorForInternalUser(subject).ifPresentOrElse(roleDescriptor -> listener.onResponse(Set.of(roleDescriptor)), () -> { + final List roleReferences = subject.getRoleReferenceIntersection(anonymousUser).getRoleReferences(); + assert roleReferences.size() == 1; // we only handle the singleton case today, but that may change with derived API keys - roleReferences.forEach( - roleReference -> roleReference.resolve( - roleReferenceResolver, - groupedActionListener.delegateFailureAndWrap((delegate, rolesRetrievalResult) -> { - if (rolesRetrievalResult.isSuccess()) { - delegate.onResponse(rolesRetrievalResult.getRoleDescriptors()); - } else { - delegate.onFailure(new ElasticsearchException("role retrieval had one or more failures")); - } - }) - ) - ); - } - ); + ActionListener.run(listener.map(rolesRetrievalResult -> { + if (rolesRetrievalResult.isSuccess() == false) { + throw new ElasticsearchException("role retrieval had one or more failures"); + } + return rolesRetrievalResult.getRoleDescriptors(); + }), l -> roleReferences.iterator().next().resolve(roleReferenceResolver, l)); + }); } // Package private for testing diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/ApiKeyUserRoleDescriptorResolverTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/ApiKeyUserRoleDescriptorResolverTests.java index 2af18f5dca7dc..502d2b7ce0dda 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/ApiKeyUserRoleDescriptorResolverTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/ApiKeyUserRoleDescriptorResolverTests.java @@ -19,8 +19,6 @@ import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; -import java.util.Collection; -import java.util.List; import java.util.Set; import java.util.stream.Collectors; @@ -56,11 +54,10 @@ public void testGetRoleDescriptors() { assertThat(subject.getType(), is(Subject.Type.USER)); assertThat(Set.of(subject.getUser().roles()), equalTo(userRoleNames)); - ActionListener>> listener = (ActionListener>>) args[args.length - - 1]; - listener.onResponse(List.of(roleDescriptors)); + ActionListener> listener = (ActionListener>) args[args.length - 1]; + listener.onResponse(roleDescriptors); return null; - }).when(rolesStore).getRoleDescriptorsList(any(Subject.class), any(ActionListener.class)); + }).when(rolesStore).getRoleDescriptors(any(Subject.class), any(ActionListener.class)); final PlainActionFuture> future = new PlainActionFuture<>(); resolver.resolveUserRoleDescriptors(authentication, future); @@ -77,6 +74,6 @@ public void testGetRoleDescriptorsEmptyForApiKey() { resolver.resolveUserRoleDescriptors(authentication, future); assertThat(future.actionGet(), equalTo(Set.of())); - verify(rolesStore, never()).getRoleDescriptorsList(any(), any()); + verify(rolesStore, never()).getRoleDescriptors(any(), any()); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java index c0df3a0947c71..4ad7c61d45d63 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java @@ -2760,15 +2760,15 @@ public void testGetRoleDescriptorsListForInternalUsers() { when(subject.getUser()).thenReturn(InternalUsers.SYSTEM_USER); final IllegalArgumentException e1 = expectThrows( IllegalArgumentException.class, - () -> compositeRolesStore.getRoleDescriptorsList(subject, new PlainActionFuture<>()) + () -> compositeRolesStore.getRoleDescriptors(subject, ActionListener.noop()) ); assertThat(e1.getMessage(), equalTo("should never try to get the roles for internal user [" + SystemUser.NAME + "]")); for (var internalUser : AuthenticationTestHelper.internalUsersWithLocalRoleDescriptor()) { when(subject.getUser()).thenReturn(internalUser); - final PlainActionFuture>> future = new PlainActionFuture<>(); - compositeRolesStore.getRoleDescriptorsList(subject, future); - assertThat(future.actionGet(), equalTo(List.of(Set.of(internalUser.getLocalClusterRoleDescriptor().get())))); + final PlainActionFuture> future = new PlainActionFuture<>(); + compositeRolesStore.getRoleDescriptors(subject, future); + assertThat(future.actionGet(), equalTo(Set.of(internalUser.getLocalClusterRoleDescriptor().get()))); } } @@ -2787,9 +2787,9 @@ public void testGetRoleDescriptorsListUsesRoleStoreToResolveRoleWithInternalRole when(subject.getRoleReferenceIntersection(any())).thenReturn( new RoleReferenceIntersection(new RoleReference.NamedRoleReference(new String[] { roleName })) ); - final PlainActionFuture>> future = new PlainActionFuture<>(); - compositeRolesStore.getRoleDescriptorsList(subject, future); - assertThat(future.actionGet(), equalTo(List.of(Set.of(expectedRoleDescriptor)))); + final PlainActionFuture> future = new PlainActionFuture<>(); + compositeRolesStore.getRoleDescriptors(subject, future); + assertThat(future.actionGet(), equalTo(Set.of(expectedRoleDescriptor))); } private void getRoleForRoleNames(CompositeRolesStore rolesStore, Collection roleNames, ActionListener listener) {