Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport 2.x] [Refactor] Change HTTP routes for Audit and Config PUT methods #3407

Merged
merged 1 commit into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,13 @@ protected final Set<String> patchEntityNames(final JsonNode patchRequestContent)
}

protected final ValidationResult<SecurityConfiguration> processPutRequest(final RestRequest request) throws IOException {
return endpointValidator.withRequiredEntityName(nameParam(request))
.map(entityName -> loadConfigurationWithRequestContent(entityName, request))
return processPutRequest(nameParam(request), request);
}

protected final ValidationResult<SecurityConfiguration> processPutRequest(final String entityName, final RestRequest request)
throws IOException {
return endpointValidator.withRequiredEntityName(entityName)
.map(ignore -> loadConfigurationWithRequestContent(entityName, request))
.map(endpointValidator::onConfigChange)
.map(this::addEntityToConfig);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import java.util.Set;

import static org.opensearch.security.dlic.rest.api.RequestHandler.methodNotImplementedHandler;
import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage;
import static org.opensearch.security.dlic.rest.api.Responses.conflictMessage;
import static org.opensearch.security.dlic.rest.api.Responses.methodNotImplementedMessage;
import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
Expand Down Expand Up @@ -124,7 +123,7 @@
private static final List<Route> routes = addRoutesPrefix(
ImmutableList.of(
new Route(RestRequest.Method.GET, "/audit/"),
new Route(RestRequest.Method.PUT, "/audit/{name}"),
new Route(RestRequest.Method.PUT, "/audit/config"),
new Route(RestRequest.Method.PATCH, "/audit/")
)
);
Expand Down Expand Up @@ -237,6 +236,9 @@
return CType.AUDIT;
}

@Override
protected void consumeParameters(RestRequest request) {}

Check warning on line 240 in src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java#L240

Added line #L240 was not covered by tests

private void auditApiRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) {
requestHandlersBuilder.onGetRequest(
request -> withEnabledAuditApi(request).map(this::processGetRequest).map(securityConfiguration -> {
Expand All @@ -248,7 +250,7 @@
.onChangeRequest(RestRequest.Method.PATCH, request -> withEnabledAuditApi(request).map(this::processPatchRequest))
.onChangeRequest(
RestRequest.Method.PUT,
request -> withEnabledAuditApi(request).map(this::withConfigEntityNameOnly).map(ignore -> processPutRequest(request))
request -> withEnabledAuditApi(request).map(ignore -> processPutRequest("config", request))

Check warning on line 253 in src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java#L253

Added line #L253 was not covered by tests
)
.override(RestRequest.Method.POST, methodNotImplementedHandler)
.override(RestRequest.Method.DELETE, methodNotImplementedHandler);
Expand All @@ -261,14 +263,6 @@
return ValidationResult.success(request);
}

ValidationResult<String> withConfigEntityNameOnly(final RestRequest request) {
final var name = nameParam(request);
if (!"config".equals(name)) {
return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("name must be config"));
}
return ValidationResult.success(name);
}

@Override
protected EndpointValidator createEndpointValidator() {
return new EndpointValidator() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,7 @@

package org.opensearch.security.dlic.rest.api;

import java.util.Collections;
import java.util.List;
import java.util.Map;

import com.google.common.collect.ImmutableList;

import com.google.common.collect.ImmutableMap;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.inject.Inject;
Expand All @@ -32,8 +27,11 @@
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.threadpool.ThreadPool;

import java.util.Collections;
import java.util.List;
import java.util.Map;

import static org.opensearch.security.dlic.rest.api.RequestHandler.methodNotImplementedHandler;
import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage;
import static org.opensearch.security.dlic.rest.api.Responses.methodNotImplementedMessage;
import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;

Expand All @@ -43,7 +41,7 @@

private static final List<Route> allRoutes = new ImmutableList.Builder<Route>().addAll(getRoutes)
.addAll(
addRoutesPrefix(ImmutableList.of(new Route(Method.PUT, "/securityconfig/{name}"), new Route(Method.PATCH, "/securityconfig/")))
addRoutesPrefix(ImmutableList.of(new Route(Method.PUT, "/securityconfig/config"), new Route(Method.PATCH, "/securityconfig/")))
)
.build();

Expand Down Expand Up @@ -72,10 +70,13 @@
return CType.CONFIG;
}

@Override
protected void consumeParameters(RestRequest request) {}

Check warning on line 74 in src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java#L74

Added line #L74 was not covered by tests

private void securityConfigApiActionRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) {
requestHandlersBuilder.onChangeRequest(
Method.PUT,
request -> withAllowedEndpoint(request).map(this::withConfigEntityNameOnly).map(ignore -> processPutRequest(request))
request -> withAllowedEndpoint(request).map(ignore -> processPutRequest("config", request))

Check warning on line 79 in src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java#L79

Added line #L79 was not covered by tests
)
.onChangeRequest(Method.PATCH, request -> withAllowedEndpoint(request).map(this::processPatchRequest))
.override(Method.DELETE, methodNotImplementedHandler)
Expand All @@ -89,14 +90,6 @@
return ValidationResult.success(request);
}

ValidationResult<String> withConfigEntityNameOnly(final RestRequest request) {
final var name = nameParam(request);
if (!"config".equals(name)) {
return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("name must be config"));
}
return ValidationResult.success(name);
}

@Override
protected EndpointValidator createEndpointValidator() {
return new EndpointValidator() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.opensearch.security.util.FakeRestRequest;

import java.util.List;
import java.util.Map;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand Down Expand Up @@ -51,17 +50,6 @@ public void enabledAuditApi() {
}
}

@Test
public void configEntityNameOnly() {
final var auditApiAction = new AuditApiAction(clusterService, threadPool, securityApiDependencies);
var result = auditApiAction.withConfigEntityNameOnly(FakeRestRequest.builder().withParams(Map.of("name", "aaaaa")).build());
assertFalse(result.isValid());
assertEquals(RestStatus.BAD_REQUEST, result.status());

result = auditApiAction.withConfigEntityNameOnly(FakeRestRequest.builder().withParams(Map.of("name", "config")).build());
assertTrue(result.isValid());
}

@Test
public void onChangeVerifyReadonlyFields() throws Exception {
final var auditApiActionEndpointValidator = new AuditApiAction(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@

import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX;

public class SecurityConfigApiTest extends AbstractRestApiUnitTest {
public class SecurityConfigApiActionTest extends AbstractRestApiUnitTest {
private final String ENDPOINT;

protected String getEndpointPrefix() {
return PLUGINS_PREFIX;
}

public SecurityConfigApiTest() {
public SecurityConfigApiActionTest() {
ENDPOINT = getEndpointPrefix() + "/api";
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,12 @@
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.util.FakeRestRequest;

import java.util.Map;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class SecurityConfigApiActionValidationTest extends AbstractApiActionValidationTest {

@Test
public void configEntityNameOnly() {
final var securityConfigApiAction = new SecurityConfigApiAction(clusterService, threadPool, securityApiDependencies);
var result = securityConfigApiAction.withConfigEntityNameOnly(
FakeRestRequest.builder().withParams(Map.of("name", "aaaaa")).build()
);
assertFalse(result.isValid());
assertEquals(RestStatus.BAD_REQUEST, result.status());

result = securityConfigApiAction.withConfigEntityNameOnly(FakeRestRequest.builder().withParams(Map.of("name", "config")).build());
assertTrue(result.isValid());
}

@Test
public void withAllowedEndpoint() {
var securityConfigApiAction = new SecurityConfigApiAction(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@

package org.opensearch.security.dlic.rest.api.legacy;

import org.opensearch.security.dlic.rest.api.SecurityConfigApiTest;
import org.opensearch.security.dlic.rest.api.SecurityConfigApiActionTest;

import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX;

public class LegacySecurityConfigApiTests extends SecurityConfigApiTest {
public class LegacySecurityConfigApiActionTests extends SecurityConfigApiActionTest {
@Override
protected String getEndpointPrefix() {
return LEGACY_OPENDISTRO_PREFIX;
Expand Down
Loading