Remove magic string for the empty Role (#89766)

When a runtime Role is empty, a magic string "__empty" was used by the
existing code to label it. This magic string ends up being treated as
a role name and in turn written to the audit log (as a field of
`"user.roles": ["__empty"]`). This is wrong because it seems to suggest
that the auditted user have a role named "__empty". In addition,
"__empty" is in fact a valid role name which further adds to the
confusion.

This PR removes the magic string and simply let the empty Role to have
an empty list of role names which should correct relevant audit log
entry to `"user.roles": []`.

docs/changelog/89766.yaml

@@ -0,0 +1,5 @@
+pr: 89766
+summary: Remove magic string for the empty Role
+area: Authorization
+type: bug
+issues: []

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java

@@ -38,7 +38,7 @@ import java.util.function.Predicate;
 
 public interface Role {
 
-    Role EMPTY = builder(new RestrictedIndices(Automatons.EMPTY), "__empty").build();
+    Role EMPTY = builder(new RestrictedIndices(Automatons.EMPTY)).build();
 
     String[] names();
 

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/SimpleRoleTests.java

@@ -16,12 +16,17 @@ import java.util.Map;
 import java.util.concurrent.ExecutionException;
 
 import static org.elasticsearch.xpack.core.security.test.TestRestrictedIndices.RESTRICTED_INDICES;
+import static org.hamcrest.Matchers.emptyArray;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
 
 public class SimpleRoleTests extends ESTestCase {
 
+    public void testEmptyRoleHasNoEmptyListOfNames() {
+        assertThat(Role.EMPTY.names(), emptyArray());
+    }
+
     public void testHasPrivilegesCache() throws ExecutionException {
         final SimpleRole role = Role.builder(
             new RoleDescriptor(randomAlphaOfLengthBetween(3, 8), new String[] { "monitor" }, null, null),

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationDenialMessages.java

@@ -15,7 +15,6 @@ import org.elasticsearch.xpack.core.security.authc.Authentication;
 import org.elasticsearch.xpack.core.security.authc.AuthenticationField;
 import org.elasticsearch.xpack.core.security.authc.Subject;
 import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo;
-import org.elasticsearch.xpack.core.security.authz.permission.Role;
 import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
 import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
 
@@ -134,16 +133,16 @@ class AuthorizationDenialMessages {
         if (authorizationInfo == null) {
             return null;
         }
-        final Role role = RBACEngine.maybeGetRBACEngineRole(authorizationInfo);
-        if (role == Role.EMPTY) {
-            return List.of();
-        } else {
-            final Map<String, Object> info = authorizationInfo.asMap();
-            if (false == info.containsKey(PRINCIPAL_ROLES_FIELD_NAME)) {
-                return null;
-            }
-            return Arrays.stream((String[]) info.get(PRINCIPAL_ROLES_FIELD_NAME)).sorted().toList();
+
+        final Map<String, Object> info = authorizationInfo.asMap();
+        final Object roleNames = info.get(PRINCIPAL_ROLES_FIELD_NAME);
+        // AuthorizationInfo from custom authorization engine may not have this field or have it as a different data type
+        if (false == roleNames instanceof String[]) {
+            assert false == authorizationInfo instanceof RBACEngine.RBACAuthorizationInfo
+                : "unexpected user.roles field [" + roleNames + "] for RBACAuthorizationInfo";
+            return null;
         }
+        return Arrays.stream((String[]) roleNames).sorted().toList();
     }
 
     private static String actionIsUnauthorizedMessage(String action, String userText) {

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationDenialMessagesTests.java

@@ -8,12 +8,12 @@
 package org.elasticsearch.xpack.security.authz;
 
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.core.Tuple;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xpack.core.security.authc.Authentication;
 import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper;
 import org.elasticsearch.xpack.core.security.authc.Subject;
 import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo;
-import org.elasticsearch.xpack.core.security.authz.permission.Role;
 import org.elasticsearch.xpack.core.security.user.User;
 
 import java.util.List;
@@ -77,6 +77,37 @@ public class AuthorizationDenialMessagesTests extends ESTestCase {
         );
     }
 
+    public void testRolesDescriptionWithIncompatibleRolesField() {
+        // random 0 - 3 uniquely named roles
+        final List<String> assignedRoleNames = IntStream.range(0, randomIntBetween(0, 3))
+            .mapToObj(i -> randomAlphaOfLengthBetween(3, 8) + i)
+            .toList();
+        final Subject subject = AuthenticationTestHelper.builder()
+            .realm()
+            .user(new User(randomAlphaOfLengthBetween(3, 8), assignedRoleNames.toArray(String[]::new)))
+            .build(false)
+            .getEffectiveSubject();
+        final AuthorizationInfo authorizationInfo = mock(AuthorizationInfo.class);
+        when(authorizationInfo.asMap()).thenReturn(
+            Map.of(
+                "user.roles",
+                randomFrom(
+                    randomAlphaOfLength(8),
+                    42,
+                    randomBoolean(),
+                    randomList(3, () -> randomAlphaOfLength(8)),
+                    randomMap(0, 3, () -> Tuple.tuple(randomAlphaOfLength(5), randomAlphaOfLength(8)))
+                )
+            )
+        );
+        final String rolesDescription = AuthorizationDenialMessages.rolesDescription(subject, authorizationInfo);
+
+        assertThat(
+            rolesDescription,
+            equalTo(" with assigned roles [%s]".formatted(Strings.collectionToCommaDelimitedString(assignedRoleNames)))
+        );
+    }
+
     public void testRoleDescriptionWithEmptyResolvedRole() {
         // random 0 - 3 uniquely named roles
         final List<String> assignedRoleNames = IntStream.range(0, randomIntBetween(0, 3))
@@ -88,9 +119,9 @@ public class AuthorizationDenialMessagesTests extends ESTestCase {
             .build(false)
             .getEffectiveSubject();
 
-        final RBACEngine.RBACAuthorizationInfo rbacAuthorizationInfo = mock(RBACEngine.RBACAuthorizationInfo.class);
-        when(rbacAuthorizationInfo.getRole()).thenReturn(Role.EMPTY);
-        final String rolesDescription = AuthorizationDenialMessages.rolesDescription(subject, rbacAuthorizationInfo);
+        final AuthorizationInfo authorizationInfo = mock(AuthorizationInfo.class);
+        when(authorizationInfo.asMap()).thenReturn(Map.of("user.roles", Strings.EMPTY_ARRAY));
+        final String rolesDescription = AuthorizationDenialMessages.rolesDescription(subject, authorizationInfo);
 
         if (assignedRoleNames.isEmpty()) {
             assertThat(rolesDescription, equalTo(" with effective roles []"));

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java

@@ -7,6 +7,7 @@
 
 package org.elasticsearch.xpack.security.authz;
 
+import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthAction;
 import org.elasticsearch.action.admin.cluster.state.ClusterStateAction;
 import org.elasticsearch.action.admin.cluster.stats.ClusterStatsAction;
@@ -27,6 +28,7 @@ import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.collect.MapBuilder;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.set.Sets;
+import org.elasticsearch.core.Tuple;
 import org.elasticsearch.license.GetLicenseAction;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.transport.TransportRequest;
@@ -53,6 +55,7 @@ import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings;
 import org.elasticsearch.xpack.core.security.authc.file.FileRealmSettings;
 import org.elasticsearch.xpack.core.security.authc.ldap.LdapRealmSettings;
 import org.elasticsearch.xpack.core.security.authc.pki.PkiRealmSettings;
+import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
 import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo;
 import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.PrivilegesCheckResult;
 import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.PrivilegesToCheck;
@@ -96,10 +99,12 @@ import java.util.stream.Collectors;
 
 import static java.util.Collections.emptyMap;
 import static org.elasticsearch.common.util.set.Sets.newHashSet;
+import static org.elasticsearch.test.ActionListenerUtils.anyActionListener;
 import static org.elasticsearch.xpack.core.security.test.TestRestrictedIndices.RESTRICTED_INDICES;
 import static org.elasticsearch.xpack.security.authz.AuthorizedIndicesTests.getRequestInfo;
 import static org.hamcrest.Matchers.aMapWithSize;
 import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.emptyArray;
 import static org.hamcrest.Matchers.emptyIterable;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
@@ -126,12 +131,54 @@ import static org.mockito.Mockito.when;
 public class RBACEngineTests extends ESTestCase {
 
     private RBACEngine engine;
+    private CompositeRolesStore rolesStore;
 
     @Before
     public void createEngine() {
         final LoadAuthorizedIndicesTimeChecker.Factory timerFactory = mock(LoadAuthorizedIndicesTimeChecker.Factory.class);
         when(timerFactory.newTimer(any())).thenReturn(LoadAuthorizedIndicesTimeChecker.NO_OP_CONSUMER);
-        engine = new RBACEngine(Settings.EMPTY, mock(CompositeRolesStore.class), timerFactory);
+        rolesStore = mock(CompositeRolesStore.class);
+        engine = new RBACEngine(Settings.EMPTY, rolesStore, timerFactory);
+    }
+
+    public void testResolveAuthorizationInfoForEmptyRolesWithAuthentication() {
+        doAnswer(invocation -> {
+            @SuppressWarnings("unchecked")
+            final var listener = (ActionListener<Tuple<Role, Role>>) invocation.getArgument(1);
+            listener.onResponse(new Tuple<>(Role.EMPTY, Role.EMPTY));
+            return null;
+        }).when(rolesStore).getRoles(any(), anyActionListener());
+
+        final PlainActionFuture<AuthorizationInfo> future = new PlainActionFuture<>();
+        engine.resolveAuthorizationInfo(
+            new AuthorizationEngine.RequestInfo(
+                AuthenticationTestHelper.builder().build(),
+                mock(TransportRequest.class),
+                randomAlphaOfLengthBetween(20, 30),
+                null
+            ),
+            future
+        );
+
+        final AuthorizationInfo authorizationInfo = future.actionGet();
+        assertThat((String[]) authorizationInfo.asMap().get("user.roles"), emptyArray());
+        assertThat((String[]) authorizationInfo.getAuthenticatedUserAuthorizationInfo().asMap().get("user.roles"), emptyArray());
+    }
+
+    public void testResolveAuthorizationInfoForEmptyRoleWithSubject() {
+        doAnswer(invocation -> {
+            @SuppressWarnings("unchecked")
+            final var listener = (ActionListener<Role>) invocation.getArgument(1);
+            listener.onResponse(Role.EMPTY);
+            return null;
+        }).when(rolesStore).getRole(any(), anyActionListener());
+
+        final PlainActionFuture<AuthorizationInfo> future = new PlainActionFuture<>();
+        engine.resolveAuthorizationInfo(AuthenticationTestHelper.builder().build().getEffectiveSubject(), future);
+
+        final AuthorizationInfo authorizationInfo = future.actionGet();
+        assertThat((String[]) authorizationInfo.asMap().get("user.roles"), emptyArray());
+        assertThat((String[]) authorizationInfo.getAuthenticatedUserAuthorizationInfo().asMap().get("user.roles"), emptyArray());
     }
 
     public void testSameUserPermission() {