[8.16] Add ECK Role Mapping Cleanup (115823) (#115871)

* Merge

* Fix merge

* Versions

* Nit

---------

Co-authored-by: Johannes Fredén <[email protected]>

docs/changelog/115823.yaml

@@ -0,0 +1,5 @@
+pr: 115823
+summary: Add ECK Role Mapping Cleanup
+area: Security
+type: bug
+issues: []

qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/FileSettingsRoleMappingUpgradeIT.java

@@ -23,19 +23,20 @@
 import org.junit.rules.TemporaryFolder;
 import org.junit.rules.TestRule;
 
-import java.io.IOException;
 import java.util.List;
+import java.util.Map;
 import java.util.function.Supplier;
 
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 
 public class FileSettingsRoleMappingUpgradeIT extends ParameterizedRollingUpgradeTestCase {
 
-    private static final String settingsJSON = """
+    private static final int ROLE_MAPPINGS_CLEANUP_MIGRATION_VERSION = 2;
+    private static final String SETTING_JSON = """
         {
              "metadata": {
                  "version": "1",
@@ -53,7 +54,6 @@ public class FileSettingsRoleMappingUpgradeIT extends ParameterizedRollingUpgrad
         }""";
 
     private static final TemporaryFolder repoDirectory = new TemporaryFolder();
-
     private static final ElasticsearchCluster cluster = ElasticsearchCluster.local()
         .distribution(DistributionType.DEFAULT)
         .version(getOldClusterTestVersion())
@@ -68,7 +68,7 @@ public String get() {
         .setting("xpack.security.enabled", "true")
         // workaround to avoid having to set up clients and authorization headers
         .setting("xpack.security.authc.anonymous.roles", "superuser")
-        .configFile("operator/settings.json", Resource.fromString(settingsJSON))
+        .configFile("operator/settings.json", Resource.fromString(SETTING_JSON))
         .build();
 
     @ClassRule
@@ -87,11 +87,34 @@ protected ElasticsearchCluster getUpgradeCluster() {
     public void checkVersions() {
         assumeTrue(
             "Only relevant when upgrading from a version before role mappings were stored in cluster state",
-            oldClusterHasFeature("gte_v8.4.0") && oldClusterHasFeature("gte_v8.15.0") == false
+            oldClusterHasFeature("gte_v8.7.0") && oldClusterHasFeature("gte_v8.15.0") == false
         );
     }
 
-    public void testRoleMappingsAppliedOnUpgrade() throws IOException {
+    private static void waitForSecurityMigrationCompletionIfIndexExists() throws Exception {
+        final Request request = new Request("GET", "_cluster/state/metadata/.security-7");
+        assertBusy(() -> {
+            Map<String, Object> indices = new XContentTestUtils.JsonMapView(entityAsMap(client().performRequest(request))).get(
+                "metadata.indices"
+            );
+            assertNotNull(indices);
+            // If the security index exists, migration needs to happen. There is a bug in pre cluster state role mappings code that tries
+            // to write file based role mappings before security index manager state is recovered, this makes it look like the security
+            // index is outdated (isIndexUpToDate == false). Because we can't rely on the index being there for old versions, this check
+            // is needed.
+            if (indices.containsKey(".security-7")) {
+                // JsonMapView doesn't support . prefixed indices (splits on .)
+                @SuppressWarnings("unchecked")
+                String responseVersion = new XContentTestUtils.JsonMapView((Map<String, Object>) indices.get(".security-7")).get(
+                    "migration_version.version"
+                );
+                assertNotNull(responseVersion);
+                assertTrue(Integer.parseInt(responseVersion) >= ROLE_MAPPINGS_CLEANUP_MIGRATION_VERSION);
+            }
+        });
+    }
+
+    public void testRoleMappingsAppliedOnUpgrade() throws Exception {
         if (isOldCluster()) {
             Request clusterStateRequest = new Request("GET", "/_cluster/state/metadata");
             List<Object> roleMappings = new XContentTestUtils.JsonMapView(entityAsMap(client().performRequest(clusterStateRequest))).get(
@@ -107,11 +130,10 @@ public void testRoleMappingsAppliedOnUpgrade() throws IOException {
             ).get("metadata.role_mappings.role_mappings");
             assertThat(clusterStateRoleMappings, is(not(nullValue())));
             assertThat(clusterStateRoleMappings.size(), equalTo(1));
-
+            waitForSecurityMigrationCompletionIfIndexExists();
             assertThat(
                 entityAsMap(client().performRequest(new Request("GET", "/_security/role_mapping"))).keySet(),
-                // TODO change this to `contains` once the clean-up migration work is merged
-                hasItem("everyone_kibana-read-only-operator-mapping")
+                contains("everyone_kibana-read-only-operator-mapping")
             );
         }
     }

server/src/main/java/org/elasticsearch/cluster/metadata/ReservedStateMetadata.java

@@ -91,6 +91,21 @@ public Set<String> conflicts(String handlerName, Set<String> modified) {
         return Collections.unmodifiableSet(intersect);
     }
 
+    /**
+     * Get the reserved keys for the handler name
+     *
+     * @param handlerName handler name to get keys for
+     * @return set of keys for that handler
+     */
+    public Set<String> keys(String handlerName) {
+        ReservedStateHandlerMetadata handlerMetadata = handlers.get(handlerName);
+        if (handlerMetadata == null || handlerMetadata.keys().isEmpty()) {
+            return Collections.emptySet();
+        }
+
+        return Collections.unmodifiableSet(handlerMetadata.keys());
+    }
+
     /**
      * Reads an {@link ReservedStateMetadata} from a {@link StreamInput}
      *

server/src/main/java/org/elasticsearch/index/IndexVersions.java

@@ -118,6 +118,7 @@ private static IndexVersion def(int id, Version luceneVersion) {
     public static final IndexVersion MERGE_ON_RECOVERY_VERSION = def(8_515_00_0, Version.LUCENE_9_11_1);
     public static final IndexVersion UPGRADE_TO_LUCENE_9_12 = def(8_516_00_0, Version.LUCENE_9_12_0);
     public static final IndexVersion ENABLE_IGNORE_ABOVE_LOGSDB = def(8_517_00_0, Version.LUCENE_9_12_0);
+    public static final IndexVersion ADD_ROLE_MAPPING_CLEANUP_MIGRATION = def(8_518_00_0, Version.LUCENE_9_12_0);
     /*
      * STOP! READ THIS FIRST! No, really,
      *        ____ _____ ___  ____  _        ____  _____    _    ____    _____ _   _ ___ ____    _____ ___ ____  ____ _____ _

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

@@ -206,15 +206,15 @@ public RoleMapperExpression getExpression() {
      * that match the {@link #getExpression() expression} in this mapping.
      */
     public List<String> getRoles() {
-        return Collections.unmodifiableList(roles);
+        return roles != null ? Collections.unmodifiableList(roles) : Collections.emptyList();
     }
 
     /**
      * The list of {@link RoleDescriptor roles} (specified by a {@link TemplateRoleName template} that evaluates to one or more names)
      * that should be assigned to users that match the {@link #getExpression() expression} in this mapping.
      */
     public List<TemplateRoleName> getRoleTemplates() {
-        return Collections.unmodifiableList(roleTemplates);
+        return roleTemplates != null ? Collections.unmodifiableList(roleTemplates) : Collections.emptyList();
     }
 
     /**
@@ -223,7 +223,7 @@ public List<TemplateRoleName> getRoleTemplates() {
      * This is not used within the mapping process, and does not affect whether the expression matches, nor which roles are assigned.
      */
     public Map<String, Object> getMetadata() {
-        return Collections.unmodifiableMap(metadata);
+        return metadata != null ? Collections.unmodifiableMap(metadata) : Collections.emptyMap();
     }
 
     /**
@@ -233,6 +233,15 @@ public boolean isEnabled() {
         return enabled;
     }
 
+    /**
+     * Whether this mapping is an operator defined/read only role mapping
+     */
+    public boolean isReadOnly() {
+        return metadata != null && metadata.get(ExpressionRoleMapping.READ_ONLY_ROLE_MAPPING_METADATA_FLAG) instanceof Boolean readOnly
+            ? readOnly
+            : false;
+    }
+
     @Override
     public String toString() {
         return getClass().getSimpleName() + "<" + name + " ; " + roles + "/" + roleTemplates + " = " + Strings.toString(expression) + ">";

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

@@ -191,6 +191,14 @@ public static boolean hasFallbackName(ExpressionRoleMapping expressionRoleMappin
         return expressionRoleMapping.getName().equals(FALLBACK_NAME);
     }
 
+    /**
+     * Check if any of the role mappings have a fallback name
+     * @return true if any role mappings have the fallback name
+     */
+    public boolean hasAnyMappingWithFallbackName() {
+        return roleMappings.stream().anyMatch(RoleMappingMetadata::hasFallbackName);
+    }
+
     /**
      * Parse a role mapping from XContent, restoring the name from a reserved metadata field.
      * Used to parse a role mapping annotated with its name in metadata via @see {@link #copyWithNameInMetadata(ExpressionRoleMapping)}.

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

@@ -204,6 +204,29 @@ public void clusterChanged(ClusterChangedEvent event) {
         return new Tuple<>(savedClusterState, metadataVersion);
     }
 
+    // Wait for any file metadata
+    public static Tuple<CountDownLatch, AtomicLong> setupClusterStateListener(String node) {
+        ClusterService clusterService = internalCluster().clusterService(node);
+        CountDownLatch savedClusterState = new CountDownLatch(1);
+        AtomicLong metadataVersion = new AtomicLong(-1);
+        clusterService.addListener(new ClusterStateListener() {
+            @Override
+            public void clusterChanged(ClusterChangedEvent event) {
+                ReservedStateMetadata reservedState = event.state().metadata().reservedStateMetadata().get(FileSettingsService.NAMESPACE);
+                if (reservedState != null) {
+                    ReservedStateHandlerMetadata handlerMetadata = reservedState.handlers().get(ReservedRoleMappingAction.NAME);
+                    if (handlerMetadata != null) {
+                        clusterService.removeListener(this);
+                        metadataVersion.set(event.state().metadata().version());
+                        savedClusterState.countDown();
+                    }
+                }
+            }
+        });
+
+        return new Tuple<>(savedClusterState, metadataVersion);
+    }
+
     public static Tuple<CountDownLatch, AtomicLong> setupClusterStateListenerForCleanup(String node) {
         ClusterService clusterService = internalCluster().clusterService(node);
         CountDownLatch savedClusterState = new CountDownLatch(1);