From ba74d1493edba694441786519fdbec5161849a16 Mon Sep 17 00:00:00 2001 From: Andrey Pleskach Date: Mon, 8 Apr 2024 14:46:23 +0200 Subject: [PATCH] Fix bug for PATCH in EndpointValidator (#4190) Signed-off-by: Andrey Pleskach --- .../dlic/rest/api/AbstractApiAction.java | 2 +- .../dlic/rest/api/RolesApiAction.java | 9 +- .../dlic/rest/api/RolesMappingApiAction.java | 11 +- .../rest/validation/EndpointValidator.java | 12 +- .../dlic/rest/api/ActionGroupsApiTest.java | 72 +++++++--- .../api/RolesApiActionValidationTest.java | 14 +- .../security/dlic/rest/api/RolesApiTest.java | 113 ++++++---------- .../RolesMappingApiActionValidationTest.java | 5 +- .../dlic/rest/api/RolesMappingApiTest.java | 125 ++++++++---------- src/test/resources/restapi/action_groups.yml | 13 ++ src/test/resources/restapi/roles.yml | 26 ++++ src/test/resources/restapi/roles_mapping.yml | 3 + 12 files changed, 214 insertions(+), 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 index a38d618da0..157d44da73 100644 --- 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 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 index 50fac9b80c..8b25fd5702 100644 --- 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 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 index b980a1e4ba..bc2e515e09 100644 --- 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 isAllowedToChangeImmutableEntity( public ValidationResult 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 index 5879272b30..17c8b1f2ba 100644 --- 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> validateRoles( default ValidationResult 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 index fb166779ac..fbeb3473fc 100644 --- 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 index 88a358dcb2..7fe089c0ba 100644 --- 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,21 +28,18 @@ 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()); @@ -50,8 +47,9 @@ public void isNotAllowedRightsToChangeImmutableEntity() throws Exception { Mockito.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()); diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/RolesApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/RolesApiTest.java index bb11bb2226..eb3ad7d4e5 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/RolesApiTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/RolesApiTest.java @@ -691,105 +691,74 @@ public void testRolesApiWithRestApiRolePermission() throws Exception { } @Test - public void testCreateOrUpdateRestApiAdminRoleForbiddenForNonSuperAdmin() throws Exception { + public void testCrudRestApiAdminRoleForbidden() 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 adminHeader = encodeBasicHeader("admin", "admin"); - final Header restApiHeader = encodeBasicHeader("test", "test"); - - final String restAdminPermissionsPayload = createRestAdminPermissionsPayload("cluster/*"); - HttpResponse response = rh.executePutRequest( - ENDPOINT + "/roles/new_rest_admin_role", - restAdminPermissionsPayload, - restApiAdminHeader - ); - Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); - response = rh.executePutRequest(ENDPOINT + "/roles/rest_admin_role_to_delete", restAdminPermissionsPayload, restApiAdminHeader); - Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); - - // attempt to create a new rest admin role by admin - response = rh.executePutRequest(ENDPOINT + "/roles/some_rest_admin_role", restAdminPermissionsPayload, adminHeader); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - - // attempt to update exiting admin role - response = rh.executePutRequest(ENDPOINT + "/roles/new_rest_admin_role", restAdminPermissionsPayload, adminHeader); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - - // attempt to patch exiting admin role - response = rh.executePatchRequest( - ENDPOINT + "/roles/new_rest_admin_role", - createPatchRestAdminPermissionsPayload("replace"), - adminHeader - ); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - - // attempt to update exiting admin role - response = rh.executePutRequest(ENDPOINT + "/roles/new_rest_admin_role", restAdminPermissionsPayload, restApiHeader); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - - // attempt to create a new rest admin role by admin - response = rh.executePutRequest(ENDPOINT + "/roles/some_rest_admin_role", restAdminPermissionsPayload, restApiHeader); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - - // attempt to patch exiting admin role and crate a new one - response = rh.executePatchRequest(ENDPOINT + "/roles", createPatchRestAdminPermissionsPayload("replace"), restApiHeader); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + final var userHeaders = List.of( + encodeBasicHeader("admin", "admin"), + encodeBasicHeader("test", "test"), + encodeBasicHeader("rest_api_admin_user", "rest_api_admin_user"), + encodeBasicHeader("rest_api_admin_roles", "rest_api_admin_roles") + ); + for (final var userHeader : userHeaders) { + final String restAdminPermissionsPayload = createRestAdminPermissionsPayload("cluster/*"); + // attempt to create a new role + verifyPutForbidden("new_rest_admin_role", restAdminPermissionsPayload, userHeader); + verifyPatchForbidden(createPatchRestAdminPermissionsPayload("new_rest_admin_role", "add"), userHeader); + + // attempt to update existing rest admin role + verifyPutForbidden("rest_api_admin_full_access", restAdminPermissionsPayload, userHeader); + verifyPatchForbidden(createPatchRestAdminPermissionsPayload("rest_api_admin_full_access", "replace"), userHeader); + + // attempt to update non rest admin role with REST admin permissions + verifyPutForbidden("opendistro_security_role_starfleet_captains", restAdminPermissionsPayload, userHeader); + verifyPatchForbidden( + createPatchRestAdminPermissionsPayload("opendistro_security_role_starfleet_captains", "replace"), + userHeader + ); + + // attempt to remove REST admin role + verifyDeleteForbidden("rest_api_admin_full_access", userHeader); + verifyPatchForbidden(createPatchRestAdminPermissionsPayload("rest_api_admin_full_access", "remove"), userHeader); + } + } - response = rh.executePatchRequest(ENDPOINT + "/roles", createPatchRestAdminPermissionsPayload("add"), restApiHeader); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - response = rh.executePatchRequest(ENDPOINT + "/roles", createPatchRestAdminPermissionsPayload("remove"), restApiHeader); + void verifyPutForbidden(final String roleName, final String restAdminPermissionsPayload, final Header... header) { + HttpResponse response = rh.executePutRequest(ENDPOINT + "/roles/" + roleName, restAdminPermissionsPayload, header); Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); } - @Test - public void testDeleteRestApiAdminRoleForbiddenForNonSuperAdmin() 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 adminHeader = encodeBasicHeader("admin", "admin"); - final Header restApiHeader = encodeBasicHeader("test", "test"); - - final String allRestAdminPermissionsPayload = createRestAdminPermissionsPayload("cluster/*"); - - HttpResponse response = rh.executePutRequest( - ENDPOINT + "/roles/new_rest_admin_role", - allRestAdminPermissionsPayload, - restApiAdminHeader - ); - Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); - - // attempt to update exiting admin role - response = rh.executeDeleteRequest(ENDPOINT + "/roles/new_rest_admin_role", adminHeader); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + void verifyPatchForbidden(final String restAdminPermissionsPayload, final Header... header) { + HttpResponse response = rh.executePatchRequest(ENDPOINT + "/roles", restAdminPermissionsPayload, header); + Assert.assertEquals(response.getBody(), HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + } - // true to change - response = rh.executeDeleteRequest(ENDPOINT + "/roles/new_rest_admin_role", allRestAdminPermissionsPayload, restApiHeader); + void verifyDeleteForbidden(final String roleName, final Header... header) { + HttpResponse response = rh.executeDeleteRequest(ENDPOINT + "/roles/" + roleName, header); Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); } - private String createPatchRestAdminPermissionsPayload(final String op) throws JsonProcessingException { + private String createPatchRestAdminPermissionsPayload(final String roleName, final String op) throws JsonProcessingException { final ArrayNode rootNode = DefaultObjectMapper.objectMapper.createArrayNode(); final ObjectNode opAddObjectNode = DefaultObjectMapper.objectMapper.createObjectNode(); final ObjectNode clusterPermissionsNode = DefaultObjectMapper.objectMapper.createObjectNode(); clusterPermissionsNode.set("cluster_permissions", clusterPermissionsForRestAdmin("cluster/*")); if ("add".equals(op)) { - opAddObjectNode.put("op", "add").put("path", "/some_rest_admin_role").set("value", clusterPermissionsNode); + opAddObjectNode.put("op", "add").put("path", "/" + roleName).set("value", clusterPermissionsNode); rootNode.add(opAddObjectNode); } if ("remove".equals(op)) { final ObjectNode opRemoveObjectNode = DefaultObjectMapper.objectMapper.createObjectNode(); - opRemoveObjectNode.put("op", "remove").put("path", "/rest_admin_role_to_delete"); + opRemoveObjectNode.put("op", "remove").put("path", "/" + roleName); rootNode.add(opRemoveObjectNode); } if ("replace".equals(op)) { final ObjectNode replaceRemoveObjectNode = DefaultObjectMapper.objectMapper.createObjectNode(); replaceRemoveObjectNode.put("op", "replace") - .put("path", "/new_rest_admin_role/cluster_permissions") + .put("path", "/" + roleName + "/cluster_permissions") .set("value", clusterPermissionsForRestAdmin("*")); rootNode.add(replaceRemoveObjectNode); diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java index 5c041989a6..f7d1d4da0b 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java @@ -35,18 +35,16 @@ public void setupRoles() throws Exception { @Test public void isAllowedRightsToChangeRoleEntity() throws Exception { - when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.ROLESMAPPING)).thenReturn(true); final var rolesMappingApiActionEndpointValidator = new RolesMappingApiAction(clusterService, threadPool, securityApiDependencies) .createEndpointValidator(); final var result = rolesMappingApiActionEndpointValidator.isAllowedToChangeImmutableEntity( - SecurityConfiguration.of("rest_api_admin_role", configuration) + SecurityConfiguration.of("rest_api_admin_role", configuration) ); assertTrue(result.isValid()); } @Test public void isNotAllowedNoRightsToChangeRoleEntity() throws Exception { - when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.ROLESMAPPING)).thenReturn(false); when(restApiAdminPrivilegesEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenCallRealMethod(); final var rolesApiActionEndpointValidator = @@ -61,7 +59,6 @@ public void isNotAllowedNoRightsToChangeRoleEntity() throws Exception { @Test public void onConfigChangeShouldCheckRoles() throws Exception { - when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.ROLESMAPPING)).thenReturn(false); when(restApiAdminPrivilegesEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenCallRealMethod(); when(configurationRepository.getConfigurationsFromIndex(List.of(CType.ROLES), false)) .thenReturn(Map.of(CType.ROLES, rolesConfiguration)); diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiTest.java index 077c852466..dc2ba33e6e 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiTest.java @@ -14,6 +14,7 @@ import java.util.List; import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; import org.apache.hc.core5.http.Header; @@ -558,97 +559,83 @@ void verifyNonSuperAdminUser(final Header[] header) throws Exception { } @Test - public void testChangeRestApiAdminRoleMappingForbiddenForNonSuperAdmin() throws Exception { + public void testChangeRestApiAdminRoleMappingForbidden() 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 adminHeader = encodeBasicHeader("admin", "admin"); - final Header restApiHeader = encodeBasicHeader("test", "test"); - - HttpResponse response = rh.executePutRequest( - ENDPOINT + "/roles/new_rest_api_role", - createRestAdminPermissionsPayload(), - restApiAdminHeader - ); - Assert.assertEquals(response.getBody(), HttpStatus.SC_CREATED, response.getStatusCode()); - response = rh.executePutRequest( - ENDPOINT + "/roles/new_rest_api_role_without_mapping", - createRestAdminPermissionsPayload(), - restApiAdminHeader + final var userHeaders = List.of( + encodeBasicHeader("admin", "admin"), + encodeBasicHeader("test", "test"), + encodeBasicHeader("rest_api_admin_user", "rest_api_admin_user"), + encodeBasicHeader("rest_api_admin_rolesmapping", "rest_api_admin_rolesmapping") ); - Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); - response = rh.executePutRequest( - ENDPOINT + "/rolesmapping/new_rest_api_role", - createUsersPayload("a", "b", "c"), - restApiAdminHeader - ); - Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); - verifyRestApiPutAndDeleteForNonRestApiAdmin(adminHeader); - verifyRestApiPutAndDeleteForNonRestApiAdmin(restApiHeader); - verifyRestApiPatchForNonRestApiAdmin(adminHeader, false); - verifyRestApiPatchForNonRestApiAdmin(restApiHeader, false); - verifyRestApiPatchForNonRestApiAdmin(adminHeader, true); - verifyRestApiPatchForNonRestApiAdmin(restApiHeader, true); - } + for (final var userHeader : userHeaders) { + // create new mapping for existing group + verifyPutForbidden("rest_api_admin_roles_mapping_test_without_mapping", createUsers("c", "d"), userHeader); + verifyPatchForbidden(createPatchPayload("rest_api_admin_roles_mapping_test_without_mapping", "add"), userHeader); - private void verifyRestApiPutAndDeleteForNonRestApiAdmin(final Header header) throws Exception { - HttpResponse response = rh.executePutRequest( - ENDPOINT + "/rolesmapping/new_rest_api_role", - createUsersPayload("a", "b", "c"), - header - ); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + // update existing mapping with additional users + verifyPutForbidden("rest_api_admin_roles_mapping_test_with_mapping", createUsers("c", "d"), userHeader); + verifyPatchForbidden(createPatchPayload("rest_api_admin_roles_mapping_test_with_mapping", "replace"), userHeader); - response = rh.executeDeleteRequest(ENDPOINT + "/rolesmapping/new_rest_api_role", "", header); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + // delete existing role mapping forbidden + verifyDeleteForbidden("rest_api_admin_roles_mapping_test_with_mapping", userHeader); + verifyPatchForbidden(createPatchPayload("rest_api_admin_roles_mapping_test_with_mapping", "remove"), userHeader); + } } - private void verifyRestApiPatchForNonRestApiAdmin(final Header header, boolean bulk) throws Exception { - String path = ENDPOINT + "/rolesmapping"; - if (!bulk) { - path += "/new_rest_api_role"; - } - HttpResponse response = rh.executePatchRequest(path, createPathPayload("add"), header); - System.err.println(response.getBody()); + void verifyPutForbidden(final String roleMappingName, final String payload, final Header... header) { + HttpResponse response = rh.executePutRequest(ENDPOINT + "/rolesmapping/" + roleMappingName, payload, header); Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + } - response = rh.executePatchRequest(path, createPathPayload("replace"), header); + void verifyPatchForbidden(final String payload, final Header... header) { + HttpResponse response = rh.executePatchRequest(ENDPOINT + "/rolesmapping", payload, header); Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + } - response = rh.executePatchRequest(path, createPathPayload("remove"), header); + void verifyDeleteForbidden(final String roleMappingName, final Header... header) { + HttpResponse response = rh.executeDeleteRequest(ENDPOINT + "/rolesmapping/" + roleMappingName, header); Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); } - private ObjectNode createUsersObjectNode(final String... users) { - final ArrayNode usersArray = DefaultObjectMapper.objectMapper.createArrayNode(); - for (final String user : users) { - usersArray.add(user); + private String createPatchPayload(final String roleName, final String op) throws JsonProcessingException { + final ArrayNode rootNode = DefaultObjectMapper.objectMapper.createArrayNode(); + final ObjectNode opAddObjectNode = DefaultObjectMapper.objectMapper.createObjectNode(); + final ObjectNode clusterPermissionsNode = DefaultObjectMapper.objectMapper.createObjectNode(); + clusterPermissionsNode.set("users", createUsersArray("c", "d")); + if ("add".equals(op)) { + opAddObjectNode.put("op", "add").put("path", "/" + roleName).set("value", clusterPermissionsNode); + rootNode.add(opAddObjectNode); } - return DefaultObjectMapper.objectMapper.createObjectNode().set("users", usersArray); - } - private String createUsersPayload(final String... users) throws JsonProcessingException { - return DefaultObjectMapper.objectMapper.writeValueAsString(createUsersObjectNode(users)); - } - - private String createPathPayload(final String op) throws JsonProcessingException { - final ArrayNode arrayNode = DefaultObjectMapper.objectMapper.createArrayNode(); - final ObjectNode opNode = DefaultObjectMapper.objectMapper.createObjectNode(); - opNode.put("op", op); - if ("add".equals(op)) { - opNode.put("path", "/new_rest_api_role_without_mapping"); - opNode.set("value", createUsersObjectNode("d", "e", "f")); + if ("remove".equals(op)) { + final ObjectNode opRemoveObjectNode = DefaultObjectMapper.objectMapper.createObjectNode(); + opRemoveObjectNode.put("op", "remove").put("path", "/" + roleName); + rootNode.add(opRemoveObjectNode); } + if ("replace".equals(op)) { - opNode.put("path", "/new_rest_api_role"); - opNode.set("value", createUsersObjectNode("g", "h", "i")); + final ObjectNode replaceRemoveObjectNode = DefaultObjectMapper.objectMapper.createObjectNode(); + replaceRemoveObjectNode.put("op", "replace").put("path", "/" + roleName + "/users").set("value", createUsersArray("c", "d")); + + rootNode.add(replaceRemoveObjectNode); } - if ("remove".equals(op)) { - opNode.put("path", "/new_rest_api_role"); + return DefaultObjectMapper.objectMapper.writeValueAsString(rootNode); + } + + private String createUsers(final String... users) throws JsonProcessingException { + final var o = DefaultObjectMapper.objectMapper.createObjectNode().set("users", createUsersArray("c", "d")); + return DefaultObjectMapper.writeValueAsString(o, false); + } + + private JsonNode createUsersArray(final String... users) { + final ArrayNode usersArray = DefaultObjectMapper.objectMapper.createArrayNode(); + for (final String user : users) { + usersArray.add(user); } - return DefaultObjectMapper.objectMapper.writeValueAsString(arrayNode.add(opNode)); + return usersArray; } @Test diff --git a/src/test/resources/restapi/action_groups.yml b/src/test/resources/restapi/action_groups.yml index 638f65f72f..4ec858a69e 100644 --- a/src/test/resources/restapi/action_groups.yml +++ b/src/test/resources/restapi/action_groups.yml @@ -132,3 +132,16 @@ OPENDISTRO_SECURITY_SUGGEST: - "indices:data/read/suggest*" type: "index" description: "Migrated from v6" +rest_admin_action_group: + allowed_actions: + - 'restapi:admin/actiongroups' + - 'restapi:admin/allowlist' + - 'restapi:admin/config/update' + - 'restapi:admin/internalusers' + - 'restapi:admin/nodesdn' + - 'restapi:admin/roles' + - 'restapi:admin/rolesmapping' + - 'restapi:admin/ssl/certs/info' + - 'restapi:admin/ssl/certs/reload' + - 'restapi:admin/tenants' + type: "cluster" diff --git a/src/test/resources/restapi/roles.yml b/src/test/resources/restapi/roles.yml index 1c3756cb4d..925b051bb3 100644 --- a/src/test/resources/restapi/roles.yml +++ b/src/test/resources/restapi/roles.yml @@ -393,6 +393,32 @@ opendistro_security_role_starfleet_captains: allowed_actions: - "CRUD_UT" tenant_permissions: [] +rest_api_admin_roles_mapping_test_without_mapping: + reserved: true + cluster_permissions: + - 'restapi:admin/actiongroups' + - 'restapi:admin/allowlist' + - 'restapi:admin/config/update' + - 'restapi:admin/internalusers' + - 'restapi:admin/nodesdn' + - 'restapi:admin/roles' + - 'restapi:admin/rolesmapping' + - 'restapi:admin/ssl/certs/info' + - 'restapi:admin/ssl/certs/reload' + - 'restapi:admin/tenants' +rest_api_admin_roles_mapping_test_with_mapping: + reserved: true + cluster_permissions: + - 'restapi:admin/actiongroups' + - 'restapi:admin/allowlist' + - 'restapi:admin/config/update' + - 'restapi:admin/internalusers' + - 'restapi:admin/nodesdn' + - 'restapi:admin/roles' + - 'restapi:admin/rolesmapping' + - 'restapi:admin/ssl/certs/info' + - 'restapi:admin/ssl/certs/reload' + - 'restapi:admin/tenants' rest_api_admin_full_access: reserved: true cluster_permissions: diff --git a/src/test/resources/restapi/roles_mapping.yml b/src/test/resources/restapi/roles_mapping.yml index 8bfe826247..73673ae3ff 100644 --- a/src/test/resources/restapi/roles_mapping.yml +++ b/src/test/resources/restapi/roles_mapping.yml @@ -217,6 +217,9 @@ opendistro_security_role_host2: - "opendistro_security_host_localhost" and_backend_roles: [] description: "Migrated from v6" +rest_api_admin_roles_mapping_test_with_mapping: + reserved: true + users: [a, b] rest_api_admin_full_access: reserved: false hidden: true