Always write empty role descriptor fields to index (#110424)

* Always write empty role descriptor fields to index

muted-tests.yml

@@ -109,12 +109,6 @@ tests:
 - class: "org.elasticsearch.xpack.searchablesnapshots.FrozenSearchableSnapshotsIntegTests"
   issue: "https://github.com/elastic/elasticsearch/issues/110408"
   method: "testCreateAndRestorePartialSearchableSnapshot"
-- class: "org.elasticsearch.xpack.security.role.RoleWithDescriptionRestIT"
-  issue: "https://github.com/elastic/elasticsearch/issues/110416"
-  method: "testCreateOrUpdateRoleWithDescription"
-- class: "org.elasticsearch.xpack.security.role.RoleWithDescriptionRestIT"
-  issue: "https://github.com/elastic/elasticsearch/issues/110417"
-  method: "testCreateOrUpdateRoleWithDescription"
 - class: org.elasticsearch.test.rest.yaml.CcsCommonYamlTestSuiteIT
   method: test {p0=search.vectors/41_knn_search_half_byte_quantized/Test create, merge, and search cosine}
   issue: https://github.com/elastic/elasticsearch/issues/109978

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

@@ -86,7 +86,7 @@ public final class QueryRoleResponse extends ActionResponse implements ToXConten
             // other details of the role descriptor (in the same object).
             assert Strings.isNullOrEmpty(roleDescriptor.getName()) == false;
             builder.field("name", roleDescriptor.getName());
-            roleDescriptor.innerToXContent(builder, params, false, false);
+            roleDescriptor.innerToXContent(builder, params, false);
             if (sortValues != null && sortValues.length > 0) {
                 builder.array("_sort", sortValues);
             }

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

@@ -417,13 +417,8 @@ public class RoleDescriptor implements ToXContentObject, Writeable {
     }
 
     public XContentBuilder toXContent(XContentBuilder builder, Params params, boolean docCreation) throws IOException {
-        return toXContent(builder, params, docCreation, false);
-    }
-
-    public XContentBuilder toXContent(XContentBuilder builder, Params params, boolean docCreation, boolean includeMetadataFlattened)
-        throws IOException {
         builder.startObject();
-        innerToXContent(builder, params, docCreation, includeMetadataFlattened);
+        innerToXContent(builder, params, docCreation);
         return builder.endObject();
     }
 
@@ -435,12 +430,10 @@ public class RoleDescriptor implements ToXContentObject, Writeable {
      * @param docCreation {@code true} if the x-content is being generated for creating a document
      *                    in the security index, {@code false} if the x-content being generated
      *                    is for API display purposes
-     * @param includeMetadataFlattened {@code true} if the metadataFlattened field should be included in doc
      * @return x-content builder
      * @throws IOException if there was an error writing the x-content to the builder
      */
-    public XContentBuilder innerToXContent(XContentBuilder builder, Params params, boolean docCreation, boolean includeMetadataFlattened)
-        throws IOException {
+    public XContentBuilder innerToXContent(XContentBuilder builder, Params params, boolean docCreation) throws IOException {
         builder.array(Fields.CLUSTER.getPreferredName(), clusterPrivileges);
         if (configurableClusterPrivileges.length != 0) {
             builder.field(Fields.GLOBAL.getPreferredName());
@@ -452,9 +445,7 @@ public class RoleDescriptor implements ToXContentObject, Writeable {
             builder.array(Fields.RUN_AS.getPreferredName(), runAs);
         }
         builder.field(Fields.METADATA.getPreferredName(), metadata);
-        if (includeMetadataFlattened) {
-            builder.field(Fields.METADATA_FLATTENED.getPreferredName(), metadata);
-        }
+
         if (docCreation) {
             builder.field(Fields.TYPE.getPreferredName(), ROLE_TYPE);
         } else {
@@ -1196,7 +1187,7 @@ public class RoleDescriptor implements ToXContentObject, Writeable {
 
     public static final class RemoteIndicesPrivileges implements Writeable, ToXContentObject {
 
-        private static final RemoteIndicesPrivileges[] NONE = new RemoteIndicesPrivileges[0];
+        public static final RemoteIndicesPrivileges[] NONE = new RemoteIndicesPrivileges[0];
 
         private final IndicesPrivileges indicesPrivileges;
         private final String[] remoteClusters;

x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/role/BulkPutRoleRestIT.java

@@ -181,15 +181,74 @@ public class BulkPutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
     public void testBulkUpdates() throws Exception {
         String request = """
             {"roles": {"test1": {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["all"]}]}, "test2":
-            {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["read"]}]}, "test3":
-            {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["write"]}]}}}""";
-
+            {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["read"]}], "description": "something"}, "test3":
+            {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["write"]}], "remote_indices":[{"names":["logs-*"],
+            "privileges":["read"],"clusters":["my_cluster*","other_cluster"]}]}}}""";
         {
             Map<String, Object> responseMap = upsertRoles(request);
             assertThat(responseMap, not(hasKey("errors")));
 
             List<Map<String, Object>> items = (List<Map<String, Object>>) responseMap.get("created");
             assertEquals(3, items.size());
+
+            fetchRoleAndAssertEqualsExpected(
+                "test1",
+                new RoleDescriptor(
+                    "test1",
+                    new String[] { "all" },
+                    new RoleDescriptor.IndicesPrivileges[] {
+                        RoleDescriptor.IndicesPrivileges.builder().indices("*").privileges("all").build() },
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null
+                )
+            );
+            fetchRoleAndAssertEqualsExpected(
+                "test2",
+                new RoleDescriptor(
+                    "test2",
+                    new String[] { "all" },
+                    new RoleDescriptor.IndicesPrivileges[] {
+                        RoleDescriptor.IndicesPrivileges.builder().indices("*").privileges("read").build() },
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    "something"
+                )
+            );
+            fetchRoleAndAssertEqualsExpected(
+                "test3",
+                new RoleDescriptor(
+                    "test3",
+                    new String[] { "all" },
+                    new RoleDescriptor.IndicesPrivileges[] {
+                        RoleDescriptor.IndicesPrivileges.builder().indices("*").privileges("write").build() },
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    new RoleDescriptor.RemoteIndicesPrivileges[] {
+                        RoleDescriptor.RemoteIndicesPrivileges.builder("my_cluster*", "other_cluster")
+                            .indices("logs-*")
+                            .privileges("read")
+                            .build() },
+                    null,
+                    null,
+                    null
+                )
+            );
         }
         {
             Map<String, Object> responseMap = upsertRoles(request);
@@ -200,7 +259,7 @@ public class BulkPutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
         }
         {
             request = """
-                {"roles": {"test1": {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["read"]}]}, "test2":
+                {"roles": {"test1": {}, "test2":
                 {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["all"]}]}, "test3":
                 {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["all"]}]}}}""";
 
@@ -208,6 +267,49 @@ public class BulkPutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
             assertThat(responseMap, not(hasKey("errors")));
             List<Map<String, Object>> items = (List<Map<String, Object>>) responseMap.get("updated");
             assertEquals(3, items.size());
+
+            assertThat(responseMap, not(hasKey("errors")));
+
+            fetchRoleAndAssertEqualsExpected(
+                "test1",
+                new RoleDescriptor("test1", null, null, null, null, null, null, null, null, null, null, null)
+            );
+            fetchRoleAndAssertEqualsExpected(
+                "test2",
+                new RoleDescriptor(
+                    "test2",
+                    new String[] { "all" },
+                    new RoleDescriptor.IndicesPrivileges[] {
+                        RoleDescriptor.IndicesPrivileges.builder().indices("*").privileges("all").build() },
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null
+                )
+            );
+            fetchRoleAndAssertEqualsExpected(
+                "test3",
+                new RoleDescriptor(
+                    "test3",
+                    new String[] { "all" },
+                    new RoleDescriptor.IndicesPrivileges[] {
+                        RoleDescriptor.IndicesPrivileges.builder().indices("*").privileges("all").build() },
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null
+                )
+            );
         }
     }
 }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

@@ -59,6 +59,7 @@ import org.elasticsearch.xpack.core.security.action.role.QueryRoleResponse.Query
 import org.elasticsearch.xpack.core.security.action.role.RoleDescriptorRequestValidator;
 import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
 import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges;
+import org.elasticsearch.xpack.core.security.authz.permission.RemoteClusterPermissions;
 import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult;
 import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator;
 import org.elasticsearch.xpack.core.security.support.NativeRealmValidationUtil;
@@ -607,16 +608,41 @@ public class NativeRolesStore implements BiConsumer<Set<String>, ActionListener<
         return client.prepareDelete(SECURITY_MAIN_ALIAS, getIdForRole(roleName)).request();
     }
 
-    private XContentBuilder createRoleXContentBuilder(RoleDescriptor role) throws IOException {
+    // Package private for testing
+    XContentBuilder createRoleXContentBuilder(RoleDescriptor role) throws IOException {
         assert NativeRealmValidationUtil.validateRoleName(role.getName(), false) == null
             : "Role name was invalid or reserved: " + role.getName();
         assert false == role.hasRestriction() : "restriction is not supported for native roles";
-        return role.toXContent(
-            jsonBuilder(),
-            ToXContent.EMPTY_PARAMS,
-            true,
-            featureService.clusterHasFeature(clusterService.state(), SECURITY_ROLES_METADATA_FLATTENED)
-        );
+
+        XContentBuilder builder = jsonBuilder().startObject();
+        role.innerToXContent(builder, ToXContent.EMPTY_PARAMS, true);
+
+        if (featureService.clusterHasFeature(clusterService.state(), SECURITY_ROLES_METADATA_FLATTENED)) {
+            builder.field(RoleDescriptor.Fields.METADATA_FLATTENED.getPreferredName(), role.getMetadata());
+        }
+
+        // When role descriptor XContent is generated for the security index all empty fields need to have default values to make sure
+        // existing values are overwritten if not present since the request to update could be an UpdateRequest
+        // (update provided fields in existing document or create document) or IndexRequest (replace and reindex document)
+        if (role.hasConfigurableClusterPrivileges() == false) {
+            builder.startObject(RoleDescriptor.Fields.GLOBAL.getPreferredName()).endObject();
+        }
+
+        if (role.hasRemoteIndicesPrivileges() == false) {
+            builder.field(RoleDescriptor.Fields.REMOTE_INDICES.getPreferredName(), RoleDescriptor.RemoteIndicesPrivileges.NONE);
+        }
+
+        if (role.hasRemoteClusterPermissions() == false
+            && clusterService.state().getMinTransportVersion().onOrAfter(ROLE_REMOTE_CLUSTER_PRIVS)) {
+            builder.array(RoleDescriptor.Fields.REMOTE_CLUSTER.getPreferredName(), RemoteClusterPermissions.NONE);
+        }
+        if (role.hasDescription() == false
+            && clusterService.state().getMinTransportVersion().onOrAfter(TransportVersions.SECURITY_ROLE_DESCRIPTION)) {
+            builder.field(RoleDescriptor.Fields.DESCRIPTION.getPreferredName(), "");
+        }
+
+        builder.endObject();
+        return builder;
     }
 
     public void usageStats(ActionListener<Map<String, Object>> listener) {

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreTests.java

@@ -55,6 +55,7 @@ import org.elasticsearch.test.TransportVersionUtils;
 import org.elasticsearch.threadpool.TestThreadPool;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.xcontent.NamedXContentRegistry;
+import org.elasticsearch.xcontent.ParseField;
 import org.elasticsearch.xcontent.ToXContent;
 import org.elasticsearch.xcontent.XContentBuilder;
 import org.elasticsearch.xcontent.XContentType;
@@ -78,6 +79,7 @@ import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
 
 import java.io.IOException;
+import java.lang.reflect.Field;
 import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -138,7 +140,7 @@ public class NativeRolesStoreTests extends ESTestCase {
 
     private NativeRolesStore createRoleStoreForTest(Settings settings) {
         new ReservedRolesStore(Set.of("superuser"));
-        final ClusterService clusterService = mock(ClusterService.class);
+        final ClusterService clusterService = mockClusterServiceWithMinNodeVersion(TransportVersion.current());
         final SecuritySystemIndices systemIndices = new SecuritySystemIndices(settings);
         final FeatureService featureService = mock(FeatureService.class);
         systemIndices.init(client, featureService, clusterService);
@@ -807,6 +809,62 @@ public class NativeRolesStoreTests extends ESTestCase {
         verify(client, times(0)).bulk(any(BulkRequest.class), any());
     }
 
+    /**
+     * Make sure all top level fields for a RoleDescriptor have default values to make sure they can be set to empty in an upsert
+     * call to the roles API
+     */
+    public void testAllTopFieldsHaveEmptyDefaultsForUpsert() throws IOException, IllegalAccessException {
+        final NativeRolesStore rolesStore = createRoleStoreForTest();
+        RoleDescriptor allNullDescriptor = new RoleDescriptor(
+            "all-null-descriptor",
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null
+        );
+
+        Set<ParseField> fieldsWithoutDefaultValue = Set.of(
+            RoleDescriptor.Fields.INDEX,
+            RoleDescriptor.Fields.NAMES,
+            RoleDescriptor.Fields.ALLOW_RESTRICTED_INDICES,
+            RoleDescriptor.Fields.RESOURCES,
+            RoleDescriptor.Fields.QUERY,
+            RoleDescriptor.Fields.PRIVILEGES,
+            RoleDescriptor.Fields.CLUSTERS,
+            RoleDescriptor.Fields.APPLICATION,
+            RoleDescriptor.Fields.FIELD_PERMISSIONS,
+            RoleDescriptor.Fields.FIELD_PERMISSIONS_2X,
+            RoleDescriptor.Fields.GRANT_FIELDS,
+            RoleDescriptor.Fields.EXCEPT_FIELDS,
+            RoleDescriptor.Fields.METADATA_FLATTENED,
+            RoleDescriptor.Fields.TRANSIENT_METADATA,
+            RoleDescriptor.Fields.RESTRICTION,
+            RoleDescriptor.Fields.WORKFLOWS
+        );
+
+        String serializedOutput = Strings.toString(rolesStore.createRoleXContentBuilder(allNullDescriptor));
+        Field[] fields = RoleDescriptor.Fields.class.getFields();
+
+        for (Field field : fields) {
+            ParseField fieldValue = (ParseField) field.get(null);
+            if (fieldsWithoutDefaultValue.contains(fieldValue) == false) {
+                assertThat(
+                    "New RoleDescriptor field without a default value detected. "
+                        + "Set a value or add to excluded list if not expected to be set to empty through role APIs",
+                    serializedOutput,
+                    containsString(fieldValue.getPreferredName())
+                );
+            }
+        }
+    }
+
     private ClusterService mockClusterServiceWithMinNodeVersion(TransportVersion transportVersion) {
         final ClusterService clusterService = mock(ClusterService.class, Mockito.RETURNS_DEEP_STUBS);
         when(clusterService.state().getMinTransportVersion()).thenReturn(transportVersion);