Fix BWC for file-settings based role mappings

docs/changelog/113900.yaml

@@ -0,0 +1,5 @@
+pr: 113900
+summary: Fix BWC for file-settings based role mappings
+area: Authentication
+type: bug
+issues: []

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

@@ -11,6 +11,7 @@
 import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
 
 import java.io.IOException;
+import java.util.Collection;
 
 /**
  * Response to {@link GetRoleMappingsAction get role-mappings API}.
@@ -21,6 +22,10 @@ public class GetRoleMappingsResponse extends ActionResponse {
 
     private final ExpressionRoleMapping[] mappings;
 
+    public GetRoleMappingsResponse(Collection<ExpressionRoleMapping> mappings) {
+        this(mappings.toArray(new ExpressionRoleMapping[0]));
+    }
+
     public GetRoleMappingsResponse(ExpressionRoleMapping... mappings) {
         this.mappings = mappings;
     }

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

@@ -12,6 +12,7 @@
 import org.elasticsearch.cluster.AbstractNamedDiffable;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.NamedDiff;
+import org.elasticsearch.cluster.block.ClusterBlockLevel;
 import org.elasticsearch.cluster.metadata.Metadata;
 import org.elasticsearch.common.collect.Iterators;
 import org.elasticsearch.common.io.stream.StreamInput;
@@ -57,6 +58,7 @@ public final class RoleMappingMetadata extends AbstractNamedDiffable<Metadata.Cu
     private static final RoleMappingMetadata EMPTY = new RoleMappingMetadata(Set.of());
 
     public static RoleMappingMetadata getFromClusterState(ClusterState clusterState) {
+        clusterState.blocks().globalBlockedRaiseException(ClusterBlockLevel.READ);
         return clusterState.metadata().custom(RoleMappingMetadata.TYPE, RoleMappingMetadata.EMPTY);
     }
 

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

@@ -34,6 +34,7 @@
 import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingAction;
 import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingRequest;
 import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingRequestBuilder;
+import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingResponse;
 import org.elasticsearch.xpack.core.security.authc.RealmConfig;
 import org.elasticsearch.xpack.core.security.authc.support.UserRoleMapper;
 import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
@@ -54,6 +55,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Consumer;
+import java.util.stream.Collectors;
 
 import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
 import static org.elasticsearch.xcontent.XContentType.JSON;
@@ -63,7 +65,6 @@
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
-import static org.hamcrest.Matchers.emptyArray;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.notNullValue;
@@ -270,18 +271,25 @@ private void assertRoleMappingsSaveOK(CountDownLatch savedClusterState, AtomicLo
             assertThat(resolveRolesFuture.get(), containsInAnyOrder("kibana_user", "fleet_user"));
         }
 
-        // the role mappings are not retrievable by the role mapping action (which only accesses "native" i.e. index-based role mappings)
-        var request = new GetRoleMappingsRequest();
-        request.setNames("everyone_kibana", "everyone_fleet");
-        var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
-        assertFalse(response.hasMappings());
-        assertThat(response.mappings(), emptyArray());
-
-        // role mappings (with the same names) can also be stored in the "native" store
-        var putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana")).actionGet();
-        assertTrue(putRoleMappingResponse.isCreated());
-        putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet();
-        assertTrue(putRoleMappingResponse.isCreated());
+        // the role mappings are retrievable by the role mapping action for BWC
+        assertGetResponseHasMappings("everyone_kibana", "everyone_fleet");
+
+        // role mappings (with the same names) can be stored in the "native" store
+        {
+            PutRoleMappingResponse response = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana"))
+                .actionGet();
+            assertTrue(response.isCreated());
+            response = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet();
+            assertTrue(response.isCreated());
+        }
+        {
+            // deleting role mappings that exist in the native store and in cluster-state should result in success
+            var response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet();
+            assertTrue(response.isFound());
+            response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_fleet")).actionGet();
+            assertTrue(response.isFound());
+        }
+
     }
 
     public void testRoleMappingsApplied() throws Exception {
@@ -307,48 +315,78 @@ public void testRoleMappingsApplied() throws Exception {
             clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
         );
 
-        // native role mappings are not affected by the removal of the cluster-state based ones
+        // cluster-state role mapping was removed and is not returned in the API anymore
         {
             var request = new GetRoleMappingsRequest();
             request.setNames("everyone_kibana", "everyone_fleet");
             var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
-            assertTrue(response.hasMappings());
-            assertThat(
-                Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
-                containsInAnyOrder("everyone_kibana", "everyone_fleet")
-            );
+            assertFalse(response.hasMappings());
         }
 
-        // and roles are resolved based on the native role mappings
+        // no role mappings means no roles are resolved
         for (UserRoleMapper userRoleMapper : internalCluster().getInstances(UserRoleMapper.class)) {
             PlainActionFuture<Set<String>> resolveRolesFuture = new PlainActionFuture<>();
             userRoleMapper.resolveRoles(
                 new UserRoleMapper.UserData("anyUsername", null, List.of(), Map.of(), mock(RealmConfig.class)),
                 resolveRolesFuture
             );
-            assertThat(resolveRolesFuture.get(), contains("kibana_user_native"));
+            assertThat(resolveRolesFuture.get(), empty());
         }
 
         {
-            var request = new DeleteRoleMappingRequest();
-            request.setName("everyone_kibana");
-            var response = client().execute(DeleteRoleMappingAction.INSTANCE, request).get();
-            assertTrue(response.isFound());
-            request = new DeleteRoleMappingRequest();
-            request.setName("everyone_fleet");
-            response = client().execute(DeleteRoleMappingAction.INSTANCE, request).get();
-            assertTrue(response.isFound());
+            // Deleting non-existent native role mappings returns not found instead of illegal arg error since there are no clashing
+            // cluster-state role mappings
+            var response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).get();
+            assertFalse(response.isFound());
+            response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_fleet")).get();
+            assertFalse(response.isFound());
         }
+    }
 
-        // no roles are resolved now, because both native and cluster-state based stores have been cleared
-        for (UserRoleMapper userRoleMapper : internalCluster().getInstances(UserRoleMapper.class)) {
-            PlainActionFuture<Set<String>> resolveRolesFuture = new PlainActionFuture<>();
-            userRoleMapper.resolveRoles(
-                new UserRoleMapper.UserData("anyUsername", null, List.of(), Map.of(), mock(RealmConfig.class)),
-                resolveRolesFuture
-            );
-            assertThat(resolveRolesFuture.get(), empty());
+    public void testGetRoleMappings() throws Exception {
+        ensureGreen();
+
+        final List<String> nativeMappings = List.of("everyone_kibana", "_everyone_kibana", "zzz_mapping", "123_mapping");
+        for (var mapping : nativeMappings) {
+            client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest(mapping)).actionGet();
         }
+
+        var savedClusterState = setupClusterStateListener(internalCluster().getMasterName(), "everyone_kibana");
+        writeJSONFile(internalCluster().getMasterName(), testJSON, logger, versionCounter);
+        boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
+        assertTrue(awaitSuccessful);
+
+        var request = new GetRoleMappingsRequest();
+        var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
+        assertTrue(response.hasMappings());
+        assertThat(
+            Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
+            containsInAnyOrder("everyone_kibana", "everyone_kibana", "_everyone_kibana", "everyone_fleet", "zzz_mapping", "123_mapping")
+        );
+
+        // assert that cluster-state role mappings come last
+        List<Boolean> isReadOnlyFlags = Arrays.stream(response.mappings()).map(it -> {
+            Boolean isReadOnly = (Boolean) it.getMetadata().get("_read_only");
+            return Boolean.TRUE.equals(isReadOnly);
+        }).collect(Collectors.toList());
+        // first 4 are native (and first), last to cluster-state
+        assertThat(isReadOnlyFlags, contains(false, false, false, false, true, true));
+
+        // it's possible to delete overlapping native role mapping
+        assertTrue(client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet().isFound());
+
+        savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());
+        writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
+        awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
+        assertTrue(awaitSuccessful);
+
+        final ClusterStateResponse clusterStateResponse = clusterAdmin().state(
+            new ClusterStateRequest(TEST_REQUEST_TIMEOUT).waitForMetadataVersion(savedClusterState.v2().get())
+        ).get();
+
+        assertNull(
+            clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
+        );
     }
 
     public static Tuple<CountDownLatch, AtomicLong> setupClusterStateListenerForError(
@@ -433,11 +471,8 @@ public void testRoleMappingApplyWithSecurityIndexClosed() throws Exception {
             boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
             assertTrue(awaitSuccessful);
 
-            // no native role mappings exist
-            var request = new GetRoleMappingsRequest();
-            request.setNames("everyone_kibana", "everyone_fleet");
-            var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
-            assertFalse(response.hasMappings());
+            // even if index is closed, cluster-state role mappings are still returned
+            assertGetResponseHasMappings("everyone_kibana", "everyone_fleet");
 
             // cluster state settings are also applied
             var clusterStateResponse = clusterAdmin().state(
@@ -476,6 +511,12 @@ public void testRoleMappingApplyWithSecurityIndexClosed() throws Exception {
         }
     }
 
+    private DeleteRoleMappingRequest deleteRequest(String name) {
+        var request = new DeleteRoleMappingRequest();
+        request.setName(name);
+        return request;
+    }
+
     private PutRoleMappingRequest sampleRestRequest(String name) throws Exception {
         var json = """
             {
@@ -494,4 +535,12 @@ private PutRoleMappingRequest sampleRestRequest(String name) throws Exception {
             return new PutRoleMappingRequestBuilder(null).source(name, parser).request();
         }
     }
+
+    private static void assertGetResponseHasMappings(String... mappings) throws InterruptedException, ExecutionException {
+        var request = new GetRoleMappingsRequest();
+        request.setNames(mappings);
+        var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
+        assertTrue(response.hasMappings());
+        assertThat(Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(), containsInAnyOrder(mappings));
+    }
 }

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

@@ -897,6 +897,7 @@ Collection<Object> createComponents(
             reservedRealm
         );
         components.add(nativeUsersStore);
+        components.add(new PluginComponentBinding<>(ClusterStateRoleMapper.class, clusterStateRoleMapper));
         components.add(new PluginComponentBinding<>(NativeRoleMappingStore.class, nativeRoleMappingStore));
         components.add(new PluginComponentBinding<>(UserRoleMapper.class, userRoleMapper));
         components.add(reservedRealm);

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

@@ -16,17 +16,20 @@
 import org.elasticsearch.xpack.core.security.action.rolemapping.DeleteRoleMappingAction;
 import org.elasticsearch.xpack.core.security.action.rolemapping.DeleteRoleMappingRequest;
 import org.elasticsearch.xpack.core.security.action.rolemapping.DeleteRoleMappingResponse;
+import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
 import org.elasticsearch.xpack.security.authc.support.mapper.NativeRoleMappingStore;
 
 public class TransportDeleteRoleMappingAction extends HandledTransportAction<DeleteRoleMappingRequest, DeleteRoleMappingResponse> {
 
     private final NativeRoleMappingStore roleMappingStore;
+    private final ClusterStateRoleMapper clusterStateRoleMapper;
 
     @Inject
     public TransportDeleteRoleMappingAction(
         ActionFilters actionFilters,
         TransportService transportService,
-        NativeRoleMappingStore roleMappingStore
+        NativeRoleMappingStore roleMappingStore,
+        ClusterStateRoleMapper clusterStateRoleMapper
     ) {
         super(
             DeleteRoleMappingAction.NAME,
@@ -36,10 +39,15 @@ public TransportDeleteRoleMappingAction(
             EsExecutors.DIRECT_EXECUTOR_SERVICE
         );
         this.roleMappingStore = roleMappingStore;
+        this.clusterStateRoleMapper = clusterStateRoleMapper;
     }
 
     @Override
     protected void doExecute(Task task, DeleteRoleMappingRequest request, ActionListener<DeleteRoleMappingResponse> listener) {
-        roleMappingStore.deleteRoleMapping(request, listener.safeMap(DeleteRoleMappingResponse::new));
+        roleMappingStore.deleteRoleMapping(request, listener.delegateFailureAndWrap((l, found) -> {
+            // Return not-found, even if the role mapping is found in the file_settings namespace, since this delete operation
+            // is only used to delete in the native role mapping store
+            l.onResponse(new DeleteRoleMappingResponse(found));
+        }));
     }
 }

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

@@ -17,21 +17,29 @@
 import org.elasticsearch.xpack.core.security.action.rolemapping.GetRoleMappingsRequest;
 import org.elasticsearch.xpack.core.security.action.rolemapping.GetRoleMappingsResponse;
 import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
+import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
 import org.elasticsearch.xpack.security.authc.support.mapper.NativeRoleMappingStore;
 
 import java.util.Arrays;
+import java.util.Comparator;
+import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
 import java.util.Set;
+import java.util.stream.Stream;
 
 public class TransportGetRoleMappingsAction extends HandledTransportAction<GetRoleMappingsRequest, GetRoleMappingsResponse> {
 
     private final NativeRoleMappingStore roleMappingStore;
+    private final ClusterStateRoleMapper clusterStateRoleMapper;
 
     @Inject
     public TransportGetRoleMappingsAction(
         ActionFilters actionFilters,
         TransportService transportService,
-        NativeRoleMappingStore nativeRoleMappingStore
+        NativeRoleMappingStore nativeRoleMappingStore,
+        ClusterStateRoleMapper clusterStateRoleMapper
     ) {
         super(
             GetRoleMappingsAction.NAME,
@@ -41,6 +49,7 @@ public TransportGetRoleMappingsAction(
             EsExecutors.DIRECT_EXECUTOR_SERVICE
         );
         this.roleMappingStore = nativeRoleMappingStore;
+        this.clusterStateRoleMapper = clusterStateRoleMapper;
     }
 
     @Override
@@ -51,9 +60,29 @@ protected void doExecute(Task task, final GetRoleMappingsRequest request, final
         } else {
             names = new HashSet<>(Arrays.asList(request.getNames()));
         }
-        this.roleMappingStore.getRoleMappings(names, ActionListener.wrap(mappings -> {
-            ExpressionRoleMapping[] array = mappings.toArray(new ExpressionRoleMapping[mappings.size()]);
-            listener.onResponse(new GetRoleMappingsResponse(array));
+        roleMappingStore.getRoleMappings(names, ActionListener.wrap(mappings -> {
+            List<ExpressionRoleMapping> combinedRoleMappings = Stream.concat(
+                mappings.stream(),
+                clusterStateRoleMapper.getMappings(names)
+                    .stream()
+                    .map(this::cloneAndAddReadOnlyMetadataFlag)
+                    .sorted(Comparator.comparing(ExpressionRoleMapping::getName))
+            ).toList();
+            listener.onResponse(new GetRoleMappingsResponse(combinedRoleMappings));
         }, listener::onFailure));
     }
+
+    private ExpressionRoleMapping cloneAndAddReadOnlyMetadataFlag(ExpressionRoleMapping mapping) {
+        // Mark role mappings from cluster state as "_read_only" by adding a boolean to their metadata
+        Map<String, Object> metadataWithReadOnlyFlag = new HashMap<>(mapping.getMetadata());
+        metadataWithReadOnlyFlag.put("_read_only", true);
+        return new ExpressionRoleMapping(
+            mapping.getName(),
+            mapping.getExpression(),
+            mapping.getRoles(),
+            mapping.getRoleTemplates(),
+            metadataWithReadOnlyFlag,
+            mapping.isEnabled()
+        );
+    }
 }