Change read-only suffix for file based role mappings (#114205) (#114222)

This changes the suffix for a role mapping to be `-read-only` instead of
` (read only)` since the name can be used (and will be by Kibana) to get
a specific operator defined role mapping.

This also removes the suffix when checking cluster state for the
mapping. There is a slight risk that `<name>-read-only` exists both in
the native store and as `<name>` in the file based mapping. I think
that's very low risk, so didn't add any code to cover that case.

(cherry picked from commit bc8f9dc)

# Conflicts:
#	x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportDeleteRoleAction.java

Author: jfreden

x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java

@@ -359,9 +359,9 @@ public void testGetRoleMappings() throws Exception {
             Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
             containsInAnyOrder(
                 "everyone_kibana",
-                "everyone_kibana " + RESERVED_ROLE_MAPPING_SUFFIX,
+                "everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX,
                 "_everyone_kibana",
-                "everyone_fleet " + RESERVED_ROLE_MAPPING_SUFFIX,
+                "everyone_fleet" + RESERVED_ROLE_MAPPING_SUFFIX,
                 "zzz_mapping",
                 "123_mapping"
             )
@@ -378,6 +378,16 @@ public void testGetRoleMappings() throws Exception {
         // it's possible to delete overlapping native role mapping
         assertTrue(client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet().isFound());
 
+        // Fetch a specific file based role
+        request = new GetRoleMappingsRequest();
+        request.setNames("everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX);
+        response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
+        assertTrue(response.hasMappings());
+        assertThat(
+            Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
+            containsInAnyOrder("everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX)
+        );
+
         savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());
         writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
         awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
@@ -555,9 +565,7 @@ private static void assertGetResponseHasMappings(boolean readOnly, String... map
         assertThat(
             Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
             containsInAnyOrder(
-                Arrays.stream(mappings)
-                    .map(mapping -> mapping + (readOnly ? " " + RESERVED_ROLE_MAPPING_SUFFIX : ""))
-                    .toArray(String[]::new)
+                Arrays.stream(mappings).map(mapping -> mapping + (readOnly ? RESERVED_ROLE_MAPPING_SUFFIX : "")).toArray(String[]::new)
             )
         );
     }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportDeleteRoleAction.java

@@ -11,11 +11,13 @@
 import org.elasticsearch.action.support.ActionFilters;
 import org.elasticsearch.action.support.TransportAction;
 import org.elasticsearch.common.inject.Inject;
+import org.elasticsearch.common.logging.HeaderWarning;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.transport.TransportService;
 import org.elasticsearch.xpack.core.security.action.role.DeleteRoleAction;
 import org.elasticsearch.xpack.core.security.action.role.DeleteRoleRequest;
 import org.elasticsearch.xpack.core.security.action.role.DeleteRoleResponse;
+import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
 import org.elasticsearch.xpack.security.authz.ReservedRoleNameChecker;
 import org.elasticsearch.xpack.security.authz.store.NativeRolesStore;
 
@@ -24,16 +26,20 @@ public class TransportDeleteRoleAction extends TransportAction<DeleteRoleRequest
     private final NativeRolesStore rolesStore;
     private final ReservedRoleNameChecker reservedRoleNameChecker;
 
+    private final ClusterStateRoleMapper clusterStateRoleMapper;
+
     @Inject
     public TransportDeleteRoleAction(
         ActionFilters actionFilters,
         NativeRolesStore rolesStore,
         TransportService transportService,
-        ReservedRoleNameChecker reservedRoleNameChecker
+        ReservedRoleNameChecker reservedRoleNameChecker,
+        ClusterStateRoleMapper clusterStateRoleMapper
     ) {
         super(DeleteRoleAction.NAME, actionFilters, transportService.getTaskManager());
         this.rolesStore = rolesStore;
         this.reservedRoleNameChecker = reservedRoleNameChecker;
+        this.clusterStateRoleMapper = clusterStateRoleMapper;
     }
 
     @Override
@@ -44,7 +50,19 @@ protected void doExecute(Task task, DeleteRoleRequest request, ActionListener<De
         }
 
         try {
-            rolesStore.deleteRole(request, listener.safeMap(DeleteRoleResponse::new));
+            rolesStore.deleteRole(request, listener.safeMap((found) -> {
+                if (clusterStateRoleMapper.hasMapping(request.name())) {
+                    // Allow to delete a mapping with the same name in the native role mapping store as the file_settings namespace, but
+                    // add a warning header to signal to the caller that this could be a problem.
+                    HeaderWarning.addWarning(
+                        "A read only role mapping with the same name ["
+                            + request.name()
+                            + "] has been previously been defined in a configuration file. "
+                            + "The read only role mapping will still be active."
+                    );
+                }
+                return new DeleteRoleResponse(found);
+            }));
         } catch (Exception e) {
             logger.error((Supplier<?>) () -> "failed to delete role [" + request.name() + "]", e);
             listener.onFailure(e);

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/rolemapping/TransportGetRoleMappingsAction.java

@@ -25,6 +25,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper.RESERVED_ROLE_MAPPING_SUFFIX;
@@ -63,7 +64,12 @@ protected void doExecute(Task task, final GetRoleMappingsRequest request, final
         roleMappingStore.getRoleMappings(names, ActionListener.wrap(mappings -> {
             List<ExpressionRoleMapping> combinedRoleMappings = Stream.concat(
                 mappings.stream(),
-                clusterStateRoleMapper.getMappings(names)
+                clusterStateRoleMapper.getMappings(names == null ? null : names.stream().map(name -> {
+                    // If a read-only role is fetched by name including suffix, remove suffix
+                    return name.endsWith(RESERVED_ROLE_MAPPING_SUFFIX)
+                        ? name.substring(0, name.length() - RESERVED_ROLE_MAPPING_SUFFIX.length())
+                        : name;
+                }).collect(Collectors.toSet()))
                     .stream()
                     .map(this::cloneAndMarkAsReadOnly)
                     .sorted(Comparator.comparing(ExpressionRoleMapping::getName))
@@ -75,7 +81,7 @@ protected void doExecute(Task task, final GetRoleMappingsRequest request, final
     private ExpressionRoleMapping cloneAndMarkAsReadOnly(ExpressionRoleMapping mapping) {
         // Mark role mappings from cluster state as "read only" by adding a suffix to their name
         return new ExpressionRoleMapping(
-            mapping.getName() + " " + RESERVED_ROLE_MAPPING_SUFFIX,
+            mapping.getName() + RESERVED_ROLE_MAPPING_SUFFIX,
             mapping.getExpression(),
             mapping.getRoles(),
             mapping.getRoleTemplates(),

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/mapper/ClusterStateRoleMapper.java

@@ -47,7 +47,7 @@ public class ClusterStateRoleMapper extends AbstractRoleMapperClearRealmCache im
      * </ul>
      */
     public static final String CLUSTER_STATE_ROLE_MAPPINGS_ENABLED = "xpack.security.authc.cluster_state_role_mappings.enabled";
-    public static final String RESERVED_ROLE_MAPPING_SUFFIX = "(read only)";
+    public static final String RESERVED_ROLE_MAPPING_SUFFIX = "-read-only-operator-config";
     private static final Logger logger = LogManager.getLogger(ClusterStateRoleMapper.class);
 
     private final ScriptService scriptService;

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportDeleteRoleActionTests.java

@@ -19,6 +19,7 @@
 import org.elasticsearch.xpack.core.security.action.role.DeleteRoleResponse;
 import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper;
 import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
+import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
 import org.elasticsearch.xpack.security.authz.ReservedRoleNameChecker;
 import org.elasticsearch.xpack.security.authz.store.NativeRolesStore;
 import org.junit.BeforeClass;
@@ -66,7 +67,8 @@ public void testReservedRole() {
             mock(ActionFilters.class),
             rolesStore,
             transportService,
-            new ReservedRoleNameChecker.Default()
+            new ReservedRoleNameChecker.Default(),
+            mock(ClusterStateRoleMapper.class)
         );
 
         DeleteRoleRequest request = new DeleteRoleRequest();
@@ -115,7 +117,8 @@ private void testValidRole(String roleName) {
             mock(ActionFilters.class),
             rolesStore,
             transportService,
-            new ReservedRoleNameChecker.Default()
+            new ReservedRoleNameChecker.Default(),
+            mock(ClusterStateRoleMapper.class)
         );
 
         DeleteRoleRequest request = new DeleteRoleRequest();
@@ -168,7 +171,8 @@ public void testException() {
             mock(ActionFilters.class),
             rolesStore,
             transportService,
-            new ReservedRoleNameChecker.Default()
+            new ReservedRoleNameChecker.Default(),
+            mock(ClusterStateRoleMapper.class)
         );
 
         DeleteRoleRequest request = new DeleteRoleRequest();

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/rolemapping/TransportPutRoleMappingActionTests.java

@@ -29,6 +29,7 @@
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 
+import static org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper.RESERVED_ROLE_MAPPING_SUFFIX;
 import static org.hamcrest.Matchers.aMapWithSize;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.equalTo;
@@ -97,12 +98,18 @@ public void testPutMappingWithInvalidName() {
         final FieldExpression expression = new FieldExpression("username", Collections.singletonList(new FieldExpression.FieldValue("*")));
         IllegalArgumentException illegalArgumentException = expectThrows(
             IllegalArgumentException.class,
-            () -> put("anarchy (read only)", expression, "superuser", Collections.singletonMap("dumb", true))
+            () -> put("anarchy" + RESERVED_ROLE_MAPPING_SUFFIX, expression, "superuser", Collections.singletonMap("dumb", true))
         );
 
         assertThat(
             illegalArgumentException.getMessage(),
-            equalTo("Invalid mapping name [anarchy (read only)]. [(read only)] is not an allowed suffix")
+            equalTo(
+                "Invalid mapping name [anarchy"
+                    + RESERVED_ROLE_MAPPING_SUFFIX
+                    + "]. ["
+                    + RESERVED_ROLE_MAPPING_SUFFIX
+                    + "] is not an allowed suffix"
+            )
         );
     }