From 374190da79bc527ff42ab2bedf9594e173be7609 Mon Sep 17 00:00:00 2001 From: Andrey Pleskach Date: Fri, 7 Jun 2024 15:05:33 +0200 Subject: [PATCH] Adds validation for the action groups type key (#4400) Signed-off-by: Andrey Pleskach --- .../ActionGroupsRestApiIntegrationTest.java | 36 ++++++++++++------- .../dlic/rest/api/ActionGroupsApiAction.java | 36 ++++++++++++++++++- .../ActionGroupsApiActionValidationTest.java | 36 +++++++++++++++++++ 3 files changed, 95 insertions(+), 13 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/api/ActionGroupsRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/ActionGroupsRestApiIntegrationTest.java index e42ed2a447..7207ed0fe4 100644 --- a/src/integrationTest/java/org/opensearch/security/api/ActionGroupsRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/ActionGroupsRestApiIntegrationTest.java @@ -11,7 +11,6 @@ package org.opensearch.security.api; -import java.util.ArrayList; import java.util.List; import java.util.Optional; @@ -87,14 +86,7 @@ static ToXContentObject actionGroup( return (builder, params) -> { builder.startObject(); // TODO exclude in checking null value for the type - final var possibleTypes = new ArrayList(); - possibleTypes.add(TestSecurityConfig.ActionGroup.Type.INDEX.type()); - possibleTypes.add(TestSecurityConfig.ActionGroup.Type.CLUSTER.type()); - possibleTypes.add(null); - final var actionGroupType = randomFrom(possibleTypes); - if (actionGroupType != null) { - builder.field("type", actionGroupType); - } + builder.field("type", randomType()); if (allowedActions != null) { builder.field("allowed_actions"); allowedActionsArray(allowedActions).toXContent(builder, params); @@ -114,6 +106,10 @@ static ToXContentObject actionGroup( }; } + static String randomType() { + return randomFrom(List.of(TestSecurityConfig.ActionGroup.Type.CLUSTER.type(), TestSecurityConfig.ActionGroup.Type.INDEX.type())); + } + static ToXContentObject allowedActionsArray(final String... allowedActions) { return (builder, params) -> { builder.startArray(); @@ -185,6 +181,17 @@ void verifyBadRequestOperations(final TestRestClient client) throws Exception { "reference_itself cannot be an allowed_action of itself" ); + badRequestWithMessage(() -> client.putJson(apiPath("some_action_group"), (builder, params) -> { + builder.startObject().field("type", "asdasdsad").field("allowed_actions"); + allowedActionsArray("g", "f").toXContent(builder, params); + return builder.endObject(); + }), "Invalid action group type: asdasdsad. Supported types are: cluster, index."); + + assertMissingMandatoryKeys( + badRequest(() -> client.putJson(apiPath("some_action_group"), allowedActionsArray("a", "b", "c"))), + "allowed_actions" + ); + assertMissingMandatoryKeys( badRequest(() -> client.putJson(apiPath("some_action_group"), allowedActionsArray("a", "b", "c"))), "allowed_actions" @@ -198,7 +205,7 @@ void verifyBadRequestOperations(final TestRestClient client) throws Exception { assertInvalidKeys(badRequest(() -> client.putJson(apiPath("some_action_group"), unknownJsonFields)), "a,c"); assertNullValuesInArray(badRequest(() -> client.putJson(apiPath("some_action_group"), (builder, params) -> { - builder.startObject().field("allowed_actions"); + builder.startObject().field("type", randomType()).field("allowed_actions"); allowedActionsArray("g", null, "f").toXContent(builder, params); return builder.endObject(); }))); @@ -220,6 +227,11 @@ void verifyBadRequestOperations(final TestRestClient client) throws Exception { badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", allowedActionsArray("a", "b", "c"))))), "allowed_actions" ); + badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", (ToXContentObject) (builder, params) -> { + builder.startObject().field("type", "aaaa").field("allowed_actions"); + allowedActionsArray("g", "f").toXContent(builder, params); + return builder.endObject(); + })))); badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", (ToXContentObject) (builder, parameter) -> { builder.startObject(); @@ -228,7 +240,7 @@ void verifyBadRequestOperations(final TestRestClient client) throws Exception { })))); assertNullValuesInArray( badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", (ToXContentObject) (builder, params) -> { - builder.startObject().field("allowed_actions"); + builder.startObject().field("type", randomType()).field("allowed_actions"); allowedActionsArray("g", null, "f").toXContent(builder, params); return builder.endObject(); })))) @@ -242,7 +254,7 @@ void assertActionGroup(final TestRestClient.HttpResponse response, final String assertThat( response.getBody(), response.getTextFromJsonBody("/" + actionGroupName + "/type"), - oneOf(TestSecurityConfig.ActionGroup.Type.INDEX.type(), TestSecurityConfig.ActionGroup.Type.CLUSTER.type(), "") + oneOf(TestSecurityConfig.ActionGroup.Type.INDEX.type(), TestSecurityConfig.ActionGroup.Type.CLUSTER.type()) ); assertThat(response.getBody(), response.getTextArrayFromJsonBody("/" + actionGroupName + "/allowed_actions"), is(allowedActions)); } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java index 1c58e9ac6e..0b5dfd1499 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java @@ -13,6 +13,7 @@ import java.io.IOException; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; @@ -22,9 +23,11 @@ import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.inject.Inject; +import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.common.settings.Settings; import org.opensearch.core.rest.RestStatus; import org.opensearch.rest.RestRequest.Method; +import org.opensearch.security.OpenSearchSecurityPlugin; import org.opensearch.security.dlic.rest.support.Utils; import org.opensearch.security.dlic.rest.validation.EndpointValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; @@ -41,6 +44,14 @@ public class ActionGroupsApiAction extends AbstractApiAction { + private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(OpenSearchSecurityPlugin.class); + + static final String CLUSTER_TYPE = "cluster"; + + static final String INDEX_TYPE = "index"; + + static final Set ALLOWED_TYPES = Set.of(CLUSTER_TYPE, INDEX_TYPE); + private static final List routes = addRoutesPrefix( ImmutableList.of( // legacy mapping for backwards compatibility @@ -88,6 +99,7 @@ private void actionGroupsApiRequestHandlers(RequestHandler.RequestHandlersBuilde @Override protected EndpointValidator createEndpointValidator() { return new EndpointValidator() { + @Override public Endpoint endpoint() { return endpoint; @@ -100,7 +112,8 @@ public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() { @Override public ValidationResult onConfigChange(SecurityConfiguration securityConfiguration) throws IOException { - return EndpointValidator.super.onConfigChange(securityConfiguration).map(this::actionGroupNameIsNotSameAsRoleName) + return EndpointValidator.super.onConfigChange(securityConfiguration).map(this::validateType) + .map(this::actionGroupNameIsNotSameAsRoleName) .map(this::hasSelfReference); } @@ -137,6 +150,27 @@ private ValidationResult actionGroupNameIsNotSameAsRoleNa return ValidationResult.success(securityConfiguration); } + private ValidationResult validateType(final SecurityConfiguration securityConfiguration) { + final var requestContent = securityConfiguration.requestContent(); + if (requestContent.has("type") && !ALLOWED_TYPES.contains(requestContent.get("type").asText().toLowerCase(Locale.ROOT))) { + final var supportedTypesMessage = String.format("Supported types are: %s, %s.", CLUSTER_TYPE, INDEX_TYPE); + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage( + "Invalid action group type: " + requestContent.get("type").asText() + ". " + supportedTypesMessage + ) + + ); + } + if (!requestContent.has("type")) { + deprecationLogger.deprecate( + "type", + "Possibility to creation or update of action groups without type is deprecated and will be removed in next major release." + ); + } + return ValidationResult.success(securityConfiguration); + } + private ValidationResult hasSelfReference(final SecurityConfiguration securityConfiguration) throws IOException { // Prevent the case where action group references to itself in the allowed_actions. diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiActionValidationTest.java index 8fb8099783..908448f91b 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiActionValidationTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; @@ -70,6 +71,7 @@ public void onConfigChangeActionGroupHasSameNameAsRole() throws Exception { when(configuration.getVersion()).thenReturn(2); when(configuration.getImplementingClass()).thenCallRealMethod(); final var ag = objectMapper.createObjectNode() + .put("type", ActionGroupsApiAction.CLUSTER_TYPE) .set("allowed_actions", objectMapper.createArrayNode().add("indices:*")); final var actionGroupsApiActionEndpointValidator = new ActionGroupsApiAction(clusterService, threadPool, securityApiDependencies) .createEndpointValidator(); @@ -86,6 +88,7 @@ public void onConfigChangeActionGroupHasSelfReference() throws Exception { when(configuration.getVersion()).thenReturn(2); when(configuration.getImplementingClass()).thenCallRealMethod(); final var ag = objectMapper.createObjectNode() + .put("type", ActionGroupsApiAction.INDEX_TYPE) .set("allowed_actions", objectMapper.createArrayNode().add("ag")); final var actionGroupsApiActionEndpointValidator = new ActionGroupsApiAction(clusterService, threadPool, securityApiDependencies) .createEndpointValidator(); @@ -97,4 +100,37 @@ public void onConfigChangeActionGroupHasSelfReference() throws Exception { assertEquals("ag cannot be an allowed_action of itself", xContentToJsonNode(result.errorMessage()).get("message").asText()); } + @Test + public void validateInvalidType() throws Exception { + when(configuration.getCType()).thenReturn(CType.ACTIONGROUPS); + when(configuration.getVersion()).thenReturn(2); + when(configuration.getImplementingClass()).thenCallRealMethod(); + final var ag = objectMapper.createObjectNode() + .put("type", "some_type_we_know_nothing_about") + .set("allowed_actions", objectMapper.createArrayNode().add("ag")); + final var actionGroupsApiActionEndpointValidator = new ActionGroupsApiAction(clusterService, threadPool, securityApiDependencies) + .createEndpointValidator(); + + final var result = actionGroupsApiActionEndpointValidator + .onConfigChange(SecurityConfiguration.of(ag,"ag", configuration)); + assertFalse(result.isValid()); + assertEquals(RestStatus.BAD_REQUEST, result.status()); + assertEquals("Invalid action group type: some_type_we_know_nothing_about. Supported types are: cluster, index.", xContentToJsonNode(result.errorMessage()).get("message").asText()); + } + + @Test + public void passActionGroupWithoutType() throws Exception { + when(configuration.getCType()).thenReturn(CType.ACTIONGROUPS); + when(configuration.getVersion()).thenReturn(2); + when(configuration.getImplementingClass()).thenCallRealMethod(); + final var ag = objectMapper.createObjectNode() + .set("allowed_actions", objectMapper.createArrayNode().add("ag")); + final var actionGroupsApiActionEndpointValidator = new ActionGroupsApiAction(clusterService, threadPool, securityApiDependencies) + .createEndpointValidator(); + + final var result = actionGroupsApiActionEndpointValidator + .onConfigChange(SecurityConfiguration.of(ag,"some_ag", configuration)); + assertTrue(result.isValid()); + } + }