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/security/src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java

@@ -54,6 +54,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 +64,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 +270,28 @@ 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) cannot be stored in the "native" store
+        expectThrows(
+            IllegalArgumentException.class,
+            () -> client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana")).actionGet()
+        );
+        expectThrows(
+            IllegalArgumentException.class,
+            () -> client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet()
+        );
+        // deleting role mappings that don't exist in the native store but do in cluster-state also results in an error response
+        expectThrows(
+            IllegalArgumentException.class,
+            () -> client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet()
+        );
+        expectThrows(
+            IllegalArgumentException.class,
+            () -> client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_fleet")).actionGet()
+        );
+
     }
 
     public void testRoleMappingsApplied() throws Exception {
@@ -307,48 +317,79 @@ 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> isNativeFlags = Arrays.stream(response.mappings()).map(it -> {
+            // native=true is added in `sampleRestRequest` as part of test setup
+            Boolean isNative = (Boolean) it.getMetadata().get("native");
+            return Boolean.TRUE.equals(isNative);
+        }).collect(Collectors.toList());
+        // first 4 are native (and first), last to cluster-state
+        assertThat(isNativeFlags, contains(true, true, true, true, false, false));
+
+        // 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 +474,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,13 +514,20 @@ 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 = """
             {
                 "enabled": true,
                 "roles": [ "kibana_user_native" ],
                 "rules": { "field": { "username": "*" } },
                 "metadata": {
+                    "native": true,
                     "uuid" : "b9a59ba9-6b92-4be2-bb8d-02bb270cb3a7"
                 }
             }""";
@@ -494,4 +539,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,26 @@ 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) -> {
+            // TODO make sure we handle cluster-state blocks appropriately since `getMappings(...)` access cluster-state under the hood
+            if (false == found && clusterStateRoleMapper.hasMapping(request.getName())) {
+                // TODO perhaps always returning not-found if the role mapping is not found in native store is less confusing
+                l.onFailure(
+                    new IllegalArgumentException(
+                        "You attempted to delete a role mapping ["
+                            + request.getName()
+                            + "] that exists in the [file_settings] namespace. "
+                            + "Deleting role mappings in the [file_settings] namespace is not possible via API."
+                    )
+                );
+                return;
+            }
+            l.onResponse(new DeleteRoleMappingResponse(found));
+        }));
     }
 }

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

@@ -17,21 +17,27 @@
 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.ArrayList;
 import java.util.Arrays;
+import java.util.Comparator;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
 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 +47,7 @@ public TransportGetRoleMappingsAction(
             EsExecutors.DIRECT_EXECUTOR_SERVICE
         );
         this.roleMappingStore = nativeRoleMappingStore;
+        this.clusterStateRoleMapper = clusterStateRoleMapper;
     }
 
     @Override
@@ -51,9 +58,33 @@ 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 -> {
+            // TODO make sure we handle cluster-state blocks appropriately since `getMappings(...)` access cluster-state under the hood
+            // TODO add metadata marking these as originating from cluster-state/file-settings
+            final List<ExpressionRoleMapping> clusterStateRoleMappings = toOrderedList(clusterStateRoleMapper.getMappings(names));
+            if (clusterStateRoleMappings.isEmpty()) {
+                listener.onResponse(new GetRoleMappingsResponse(mappings));
+                return;
+            }
+
+            if (mappings.isEmpty()) {
+                listener.onResponse(new GetRoleMappingsResponse(clusterStateRoleMappings));
+                return;
+            }
+
+            // Native role mappings must come first to simplify consumer behavior around role mappings with duplicate names
+            final List<ExpressionRoleMapping> combined = new ArrayList<>(mappings);
+            combined.addAll(clusterStateRoleMappings);
+            listener.onResponse(new GetRoleMappingsResponse(combined));
         }, listener::onFailure));
     }
+
+    private static List<ExpressionRoleMapping> toOrderedList(Set<ExpressionRoleMapping> roleMappings) {
+        if (roleMappings.isEmpty()) {
+            return List.of();
+        }
+        final List<ExpressionRoleMapping> sortedList = new ArrayList<>(roleMappings);
+        sortedList.sort(Comparator.comparing(ExpressionRoleMapping::getName));
+        return sortedList;
+    }
 }