Skip to content

Commit

Permalink
Adds validation for the action groups type key (opensearch-project#4400)
Browse files Browse the repository at this point in the history 
Signed-off-by: Andrey Pleskach <[email protected]>
  • Loading branch information
@willyborankin
willyborankin authored Jun 7, 2024
1 parent fd6fb3f commit 374190d
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 13 deletions.
36 changes: 24 additions & 12 deletions src/integrationTest/java/org/opensearch/security/api/ActionGroupsRestApiIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

package org.opensearch.security.api;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

Expand Down Expand Up @@ -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<String>();
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);
Expand All @@ -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();
Expand Down Expand Up @@ -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"
Expand All @@ -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();
})));
Expand All @@ -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();
Expand All @@ -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();
}))))
Expand All @@ -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));
}
Expand Down
36 changes: 35 additions & 1 deletion src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

import java.io.IOException;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;

Expand All @@ -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;
Expand All @@ -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<String> ALLOWED_TYPES = Set.of(CLUSTER_TYPE, INDEX_TYPE);

private static final List<Route> routes = addRoutesPrefix(
ImmutableList.of(
// legacy mapping for backwards compatibility
Expand Down Expand Up @@ -88,6 +99,7 @@ private void actionGroupsApiRequestHandlers(RequestHandler.RequestHandlersBuilde
@Override
protected EndpointValidator createEndpointValidator() {
return new EndpointValidator() {

@Override
public Endpoint endpoint() {
return endpoint;
Expand All @@ -100,7 +112,8 @@ public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() {

@Override
public ValidationResult<SecurityConfiguration> 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);
}

Expand Down Expand Up @@ -137,6 +150,27 @@ private ValidationResult<SecurityConfiguration> actionGroupNameIsNotSameAsRoleNa
return ValidationResult.success(securityConfiguration);
}

private ValidationResult<SecurityConfiguration> 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<SecurityConfiguration> hasSelfReference(final SecurityConfiguration securityConfiguration)
throws IOException {
// Prevent the case where action group references to itself in the allowed_actions.
Expand Down
36 changes: 36 additions & 0 deletions src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiActionValidationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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());
}

}

0 comments on commit 374190d

Please sign in to comment.