Fix bug for PATCH in EndpointValidator (#4190)

Signed-off-by: Andrey Pleskach <ples@aiven.io>
opensearch-project · Apr 8, 2024 · ba74d14 · ba74d14
1 parent 02947dd
commit ba74d14
Show file tree

Hide file tree

Showing 12 changed files with 214 additions and 191 deletions.
diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java
@@ -241,7 +241,7 @@ protected ValidationResult<SecurityConfiguration> patchEntities(
             for (final var entityName : patchEntityNames(patchContent)) {
                 final var beforePatchEntity = configurationAsJson.get(entityName);
                 final var patchedEntity = patchedConfigurationAsJson.get(entityName);
-                // verify we can process exising or updated entities
+                // verify we can process existing or updated entities
                 if (beforePatchEntity != null && !Objects.equals(beforePatchEntity, patchedEntity)) {
                     final var checkEntityCanBeProcess = endpointValidator.isAllowedToChangeImmutableEntity(
                         SecurityConfiguration.of(entityName, configuration)

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/RolesApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/RolesApiAction.java
@@ -141,12 +141,9 @@ public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() {
             @Override
             public ValidationResult<SecurityConfiguration> isAllowedToChangeImmutableEntity(SecurityConfiguration securityConfiguration)
                 throws IOException {
-                return EndpointValidator.super.isAllowedToChangeImmutableEntity(securityConfiguration).map(ignore -> {
-                    if (isCurrentUserAdmin()) {
-                        return ValidationResult.success(securityConfiguration);
-                    }
-                    return isAllowedToChangeEntityWithRestAdminPermissions(securityConfiguration);
-                });
+                return EndpointValidator.super.isAllowedToChangeImmutableEntity(securityConfiguration).map(
+                    ignore -> isAllowedToChangeEntityWithRestAdminPermissions(securityConfiguration)
+                );
             }
 
             @Override

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/RolesMappingApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/RolesMappingApiAction.java
@@ -106,14 +106,11 @@ public ValidationResult<SecurityConfiguration> isAllowedToChangeImmutableEntity(
             public ValidationResult<SecurityConfiguration> isAllowedToChangeRoleMappingWithRestAdminPermissions(
                 SecurityConfiguration securityConfiguration
             ) throws IOException {
-                return loadConfiguration(CType.ROLES, false, false).map(rolesConfiguration -> {
-                    if (isCurrentUserAdmin()) {
-                        return ValidationResult.success(securityConfiguration);
-                    }
-                    return isAllowedToChangeEntityWithRestAdminPermissions(
+                return loadConfiguration(CType.ROLES, false, false).map(
+                    rolesConfiguration -> isAllowedToChangeEntityWithRestAdminPermissions(
                         SecurityConfiguration.of(securityConfiguration.entityName(), rolesConfiguration)
-                    );
-                }).map(ignore -> ValidationResult.success(securityConfiguration));
+                    )
+                ).map(ignore -> ValidationResult.success(securityConfiguration));
             }
 
             @Override

diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java
@@ -156,21 +156,19 @@ default ValidationResult<SecurityDynamicConfiguration<?>> validateRoles(
     default ValidationResult<SecurityConfiguration> isAllowedToChangeEntityWithRestAdminPermissions(
         final SecurityConfiguration securityConfiguration
     ) throws IOException {
+        final var configuration = securityConfiguration.configuration();
         if (securityConfiguration.entityExists()) {
-            final var configuration = securityConfiguration.configuration();
             final var existingEntity = configuration.getCEntry(securityConfiguration.entityName());
             if (restApiAdminPrivilegesEvaluator().containsRestApiAdminPermissions(existingEntity)) {
-
                 return ValidationResult.error(RestStatus.FORBIDDEN, forbiddenMessage("Access denied"));
             }
-        } else {
-            final var configuration = securityConfiguration.configuration();
-            final var configEntityContent = Utils.toConfigObject(
+        }
+        if (securityConfiguration.requestContent() != null) {
+            final var newConfigEntityContent = Utils.toConfigObject(
                 securityConfiguration.requestContent(),
                 configuration.getImplementingClass()
             );
-            if (restApiAdminPrivilegesEvaluator().containsRestApiAdminPermissions(configEntityContent)) {
-
+            if (restApiAdminPrivilegesEvaluator().containsRestApiAdminPermissions(newConfigEntityContent)) {
                 return ValidationResult.error(RestStatus.FORBIDDEN, forbiddenMessage("Access denied"));
             }
         }

diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java
@@ -441,25 +441,46 @@ public void testActionGroupsApiForActionGroupsRestApiAdmin() throws Exception {
     }
 
     @Test
-    public void testCreateActionGroupWithRestAdminPermissionsForbidden() throws Exception {
+    public void testCreateOrUpdateRestApiAdminActionGroupForbidden() throws Exception {
         setupWithRestRoles(Settings.builder().put(SECURITY_RESTAPI_ADMIN_ENABLED, true).build());
         rh.sendAdminCertificate = false;
-        final Header restApiAdminHeader = encodeBasicHeader("rest_api_admin_user", "rest_api_admin_user");
-        final Header restApiAdminActionGroupsHeader = encodeBasicHeader("rest_api_admin_actiongroups", "rest_api_admin_actiongroups");
-        final Header restApiHeader = encodeBasicHeader("test", "test");
+        final var userHeaders = List.of(
+            encodeBasicHeader("admin", "admin"),
+            encodeBasicHeader("test", "test"),
+            encodeBasicHeader("rest_api_admin_user", "rest_api_admin_user"),
+            encodeBasicHeader("rest_api_admin_actiongroups", "rest_api_admin_actiongroups")
+        );
+        for (final var userHeader : userHeaders) {
+            // attempt to create new action group with REST admin permissions
+            verifyPutForbidden("new_rest_api_admin_group", restAdminAllowedActions(), userHeader);
+            verifyPatchForbidden(createPatchRestAdminPermissionsPayload("new_rest_api_admin_group", "add"), userHeader);
+
+            // attempt to update existing action group which has REST admin permissions
+            verifyPutForbidden("rest_admin_action_group", restAdminAllowedActions(), userHeader);
+            verifyPatchForbidden(createPatchRestAdminPermissionsPayload("rest_admin_action_group", "replace"), userHeader);
+
+            // attempt to update existing action group with REST admin permissions
+            verifyPutForbidden("OPENDISTRO_SECURITY_CLUSTER_ALL", restAdminAllowedActions(), userHeader);
+            verifyPatchForbidden(createPatchRestAdminPermissionsPayload("OPENDISTRO_SECURITY_CLUSTER_ALL", "replace"), userHeader);
+
+            // attempt to delete
+            verifyDeleteForbidden("rest_admin_action_group", userHeader);
+            verifyPatchForbidden(createPatchRestAdminPermissionsPayload("rest_admin_action_group", "remove"), userHeader);
+        }
+    }
 
-        HttpResponse response = rh.executePutRequest(ENDPOINT + "/rest_api_admin_group", restAdminAllowedActions(), restApiAdminHeader);
-        Assert.assertEquals(response.getBody(), HttpStatus.SC_FORBIDDEN, response.getStatusCode());
-        response = rh.executePutRequest(ENDPOINT + "/rest_api_admin_group", restAdminAllowedActions(), restApiAdminActionGroupsHeader);
-        Assert.assertEquals(response.getBody(), HttpStatus.SC_FORBIDDEN, response.getStatusCode());
-        response = rh.executePutRequest(ENDPOINT + "/rest_api_admin_group", restAdminAllowedActions(), restApiHeader);
+    void verifyPutForbidden(final String actionGroupName, final String payload, final Header... header) {
+        HttpResponse response = rh.executePutRequest(ENDPOINT + "/" + actionGroupName, payload, header);
         Assert.assertEquals(response.getBody(), HttpStatus.SC_FORBIDDEN, response.getStatusCode());
+    }
 
-        response = rh.executePatchRequest(ENDPOINT, restAdminPatchBody(), restApiAdminHeader);
-        Assert.assertEquals(response.getBody(), HttpStatus.SC_FORBIDDEN, response.getStatusCode());
-        response = rh.executePatchRequest(ENDPOINT, restAdminPatchBody(), restApiAdminActionGroupsHeader);
+    void verifyPatchForbidden(final String payload, final Header... header) {
+        HttpResponse response = rh.executePatchRequest(ENDPOINT, payload, header);
         Assert.assertEquals(response.getBody(), HttpStatus.SC_FORBIDDEN, response.getStatusCode());
-        response = rh.executePatchRequest(ENDPOINT, restAdminPatchBody(), restApiHeader);
+    }
+
+    void verifyDeleteForbidden(final String actionGroupName, final Header... header) {
+        HttpResponse response = rh.executeDeleteRequest(ENDPOINT + "/" + actionGroupName, header);
         Assert.assertEquals(response.getBody(), HttpStatus.SC_FORBIDDEN, response.getStatusCode());
     }
 
@@ -469,13 +490,30 @@ String restAdminAllowedActions() throws JsonProcessingException {
         return DefaultObjectMapper.objectMapper.writeValueAsString(rootNode);
     }
 
-    String restAdminPatchBody() throws JsonProcessingException {
+    private String createPatchRestAdminPermissionsPayload(final String actionGroup, final String op) throws JsonProcessingException {
         final ArrayNode rootNode = DefaultObjectMapper.objectMapper.createArrayNode();
-        final ObjectNode opAddRootNode = DefaultObjectMapper.objectMapper.createObjectNode();
+        final ObjectNode opAddObjectNode = DefaultObjectMapper.objectMapper.createObjectNode();
         final ObjectNode allowedActionsNode = DefaultObjectMapper.objectMapper.createObjectNode();
         allowedActionsNode.set("allowed_actions", clusterPermissionsForRestAdmin("cluster/*"));
-        opAddRootNode.put("op", "add").put("path", "/rest_api_admin_group").set("value", allowedActionsNode);
-        rootNode.add(opAddRootNode);
+        if ("add".equals(op)) {
+            opAddObjectNode.put("op", "add").put("path", "/" + actionGroup).set("value", allowedActionsNode);
+            rootNode.add(opAddObjectNode);
+        }
+
+        if ("remove".equals(op)) {
+            final ObjectNode opRemoveObjectNode = DefaultObjectMapper.objectMapper.createObjectNode();
+            opRemoveObjectNode.put("op", "remove").put("path", "/" + actionGroup);
+            rootNode.add(opRemoveObjectNode);
+        }
+
+        if ("replace".equals(op)) {
+            final ObjectNode replaceRemoveObjectNode = DefaultObjectMapper.objectMapper.createObjectNode();
+            replaceRemoveObjectNode.put("op", "replace")
+                .put("path", "/" + actionGroup + "/allowed_actions")
+                .set("value", clusterPermissionsForRestAdmin("*"));
+
+            rootNode.add(replaceRemoveObjectNode);
+        }
         return DefaultObjectMapper.objectMapper.writeValueAsString(rootNode);
     }
 

diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/RolesApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/RolesApiActionValidationTest.java
@@ -28,30 +28,28 @@ public class RolesApiActionValidationTest extends AbstractApiActionValidationTes
 
     @Test
     public void isAllowedToChangeImmutableEntity() throws Exception {
-        when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.ROLES)).thenReturn(true);
-
         final var role = new RoleV7();
         role.setCluster_permissions(restApiAdminPermissions());
 
-        final var rolesApiActionEndpointValidator = new RolesApiAction(clusterService, threadPool, securityApiDependencies).createEndpointValidator();
-        final var result = rolesApiActionEndpointValidator.isAllowedToChangeImmutableEntity(SecurityConfiguration.of( "sss", configuration));
+        final var rolesApiActionEndpointValidator = new RolesApiAction(clusterService, threadPool, securityApiDependencies)
+            .createEndpointValidator();
+        final var result = rolesApiActionEndpointValidator.isAllowedToChangeImmutableEntity(SecurityConfiguration.of("sss", configuration));
 
         assertTrue(result.isValid());
     }
 
     @Test
     public void isNotAllowedRightsToChangeImmutableEntity() throws Exception {
-        when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.ROLES)).thenReturn(false);
-
         final var role = new RoleV7();
         role.setCluster_permissions(restApiAdminPermissions());
 
         when(configuration.exists("sss")).thenReturn(true);
         Mockito.<Object>when(configuration.getCEntry("sss")).thenReturn(role);
 
         when(restApiAdminPrivilegesEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenCallRealMethod();
-        final var rolesApiActionEndpointValidator = new RolesApiAction(clusterService, threadPool, securityApiDependencies).createEndpointValidator();
-        final var result = rolesApiActionEndpointValidator.isAllowedToChangeImmutableEntity(SecurityConfiguration.of( "sss", configuration));
+        final var rolesApiActionEndpointValidator = new RolesApiAction(clusterService, threadPool, securityApiDependencies)
+            .createEndpointValidator();
+        final var result = rolesApiActionEndpointValidator.isAllowedToChangeImmutableEntity(SecurityConfiguration.of("sss", configuration));
 
         assertFalse(result.isValid());
         assertEquals(RestStatus.FORBIDDEN, result.status());