Revert "Fix BWC for file-settings based role mappings (elastic#113900)" and related (elastic#114326)

Revert "Fix BWC for file-settings based role mappings (elastic#113900)" and related changes. Reverted commits:

- 763764c
- bc8f9dc
- ce07060

This is due to a bug in the above fix. We will reintroduce a paired down version of the fix in a subsequent PR.

(cherry picked from commit f389623)

# Conflicts:
#	x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java
#	x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportDeleteRoleAction.java
#	x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/rolemapping/TransportDeleteRoleMappingAction.java
#	x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/rolemapping/TransportPutRoleMappingAction.java

Author: n1v0lg

docs/changelog/113900.yaml

@@ -11,7 +11,6 @@
 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}.
@@ -22,10 +21,6 @@ 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/action/rolemapping/GetRoleMappingsResponse.java

@@ -34,7 +34,6 @@
 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;
@@ -59,11 +58,12 @@
 import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
 import static org.elasticsearch.xcontent.XContentType.JSON;
 import static org.elasticsearch.xpack.core.security.test.TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7;
-import static org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper.RESERVED_ROLE_MAPPING_SUFFIX;
 import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.contains;
 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,28 +270,21 @@ private void assertRoleMappingsSaveOK(CountDownLatch savedClusterState, AtomicLo
             assertThat(resolveRolesFuture.get(), containsInAnyOrder("kibana_user", "fleet_user"));
         }
 
-        // the role mappings are retrievable by the role mapping action for BWC
-        assertGetResponseHasMappings(true, "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());
-        }
-
+        // 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());
     }
 
-    public void testClusterStateRoleMappingsAddedThenDeleted() throws Exception {
+    public void testRoleMappingsApplied() throws Exception {
         ensureGreen();
 
         var savedClusterState = setupClusterStateListener(internalCluster().getMasterName(), "everyone_kibana");
@@ -300,12 +293,6 @@ public void testClusterStateRoleMappingsAddedThenDeleted() throws Exception {
         assertRoleMappingsSaveOK(savedClusterState.v1(), savedClusterState.v2());
         logger.info("---> cleanup cluster settings...");
 
-        {
-            // Deleting non-existent native role mappings returns not found even if they exist in config file
-            var response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).get();
-            assertFalse(response.isFound());
-        }
-
         savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());
 
         writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
@@ -320,95 +307,48 @@ public void testClusterStateRoleMappingsAddedThenDeleted() throws Exception {
             clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
         );
 
-        // cluster-state role mapping was removed and is not returned in the API anymore
+        // native role mappings are not affected by the removal of the cluster-state based ones
         {
             var request = new GetRoleMappingsRequest();
             request.setNames("everyone_kibana", "everyone_fleet");
             var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
-            assertFalse(response.hasMappings());
+            assertTrue(response.hasMappings());
+            assertThat(
+                Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
+                containsInAnyOrder("everyone_kibana", "everyone_fleet")
+            );
         }
 
-        // no role mappings means no roles are resolved
+        // and roles are resolved based on the native role mappings
         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());
+            assertThat(resolveRolesFuture.get(), contains("kibana_user_native"));
         }
-    }
 
-    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 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());
         }
 
-        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" + RESERVED_ROLE_MAPPING_SUFFIX,
-                "_everyone_kibana",
-                "everyone_fleet" + RESERVED_ROLE_MAPPING_SUFFIX,
-                "zzz_mapping",
-                "123_mapping"
-            )
-        );
-
-        int readOnlyCount = 0;
-        // assert that cluster-state role mappings come last
-        for (ExpressionRoleMapping mapping : response.mappings()) {
-            readOnlyCount = mapping.getName().endsWith(RESERVED_ROLE_MAPPING_SUFFIX) ? readOnlyCount + 1 : readOnlyCount;
+        // 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());
         }
-        // Two sourced from cluster-state
-        assertEquals(readOnlyCount, 2);
-
-        // 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);
-        assertTrue(awaitSuccessful);
-
-        final ClusterStateResponse clusterStateResponse = clusterAdmin().state(
-            new ClusterStateRequest().waitForMetadataVersion(savedClusterState.v2().get())
-        ).get();
-
-        assertNull(
-            clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
-        );
-
-        // Make sure remaining native mappings can still be fetched
-        request = new GetRoleMappingsRequest();
-        response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
-        assertTrue(response.hasMappings());
-        assertThat(
-            Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
-            containsInAnyOrder("_everyone_kibana", "zzz_mapping", "123_mapping")
-        );
     }
 
     public static Tuple<CountDownLatch, AtomicLong> setupClusterStateListenerForError(
@@ -493,8 +433,11 @@ public void testRoleMappingApplyWithSecurityIndexClosed() throws Exception {
             boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
             assertTrue(awaitSuccessful);
 
-            // even if index is closed, cluster-state role mappings are still returned
-            assertGetResponseHasMappings(true, "everyone_kibana", "everyone_fleet");
+            // 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());
 
             // cluster state settings are also applied
             var clusterStateResponse = clusterAdmin().state(new ClusterStateRequest().waitForMetadataVersion(savedClusterState.v2().get()))
@@ -532,12 +475,6 @@ 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 = """
             {
@@ -556,17 +493,4 @@ private PutRoleMappingRequest sampleRestRequest(String name) throws Exception {
             return new PutRoleMappingRequestBuilder(null).source(name, parser).request();
         }
     }
-
-    private static void assertGetResponseHasMappings(boolean readOnly, 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(
-                Arrays.stream(mappings).map(mapping -> mapping + (readOnly ? RESERVED_ROLE_MAPPING_SUFFIX : "")).toArray(String[]::new)
-            )
-        );
-    }
 }

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

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

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

@@ -11,13 +11,11 @@
 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;
 
@@ -26,20 +24,16 @@ 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,
-        ClusterStateRoleMapper clusterStateRoleMapper
+        ReservedRoleNameChecker reservedRoleNameChecker
     ) {
         super(DeleteRoleAction.NAME, actionFilters, transportService.getTaskManager());
         this.rolesStore = rolesStore;
         this.reservedRoleNameChecker = reservedRoleNameChecker;
-        this.clusterStateRoleMapper = clusterStateRoleMapper;
     }
 
     @Override
@@ -50,19 +44,7 @@ protected void doExecute(Task task, DeleteRoleRequest request, ActionListener<De
         }
 
         try {
-            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);
-            }));
+            rolesStore.deleteRole(request, listener.safeMap(DeleteRoleResponse::new));
         } 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/role/TransportDeleteRoleAction.java

@@ -10,27 +10,23 @@
 import org.elasticsearch.action.support.ActionFilters;
 import org.elasticsearch.action.support.HandledTransportAction;
 import org.elasticsearch.common.inject.Inject;
-import org.elasticsearch.common.logging.HeaderWarning;
 import org.elasticsearch.common.util.concurrent.EsExecutors;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.transport.TransportService;
 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,
-        ClusterStateRoleMapper clusterStateRoleMapper
+        NativeRoleMappingStore roleMappingStore
     ) {
         super(
             DeleteRoleMappingAction.NAME,
@@ -40,22 +36,10 @@ public TransportDeleteRoleMappingAction(
             EsExecutors.DIRECT_EXECUTOR_SERVICE
         );
         this.roleMappingStore = roleMappingStore;
-        this.clusterStateRoleMapper = clusterStateRoleMapper;
     }
 
     @Override
     protected void doExecute(Task task, DeleteRoleMappingRequest request, ActionListener<DeleteRoleMappingResponse> listener) {
-        if (clusterStateRoleMapper.hasMapping(request.getName())) {
-            // Since it's allowed to add a mapping with the same name in the native role mapping store as the file_settings namespace,
-            // a warning header is added to signal to the caller that this could be a problem.
-            HeaderWarning.addWarning(
-                "A read only role mapping with the same name ["
-                    + request.getName()
-                    + "] has been previously been defined in a configuration file. The role mapping ["
-                    + request.getName()
-                    + "] defined in the configuration file is read only, will not be deleted, and will remain active."
-            );
-        }
         roleMappingStore.deleteRoleMapping(request, listener.safeMap(DeleteRoleMappingResponse::new));
     }
 }