[Refactor] Change HTTP routes for Audit and Config PUT methods (opens…

…earch-project#3359) Change routs for audit and security configuration PUT methods. The previous configuration used the `{name}` parameter which is confusing since `config` the only allowed value for this parameter. This PR changes routes' configuration and removes useless validation for them. Signed-off-by: Andrey Pleskach <[email protected]>
willyborankin · Sep 25, 2023 · 22f00fa · 22f00fa
1 parent f09a6aa
commit 22f00fa
Show file tree

Hide file tree

Showing 7 changed files with 25 additions and 60 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
@@ -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);
     }

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java
@@ -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;
@@ -124,7 +123,7 @@ public class AuditApiAction extends AbstractApiAction {
     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/")
         )
     );
@@ -237,6 +236,9 @@ protected CType getConfigType() {
         return CType.AUDIT;
     }
 
+    @Override
+    protected void consumeParameters(RestRequest request) {}
+
     private void auditApiRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) {
         requestHandlersBuilder.onGetRequest(
             request -> withEnabledAuditApi(request).map(this::processGetRequest).map(securityConfiguration -> {
@@ -248,7 +250,7 @@ private void auditApiRequestHandlers(RequestHandler.RequestHandlersBuilder reque
             .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))
             )
             .override(RestRequest.Method.POST, methodNotImplementedHandler)
             .override(RestRequest.Method.DELETE, methodNotImplementedHandler);
@@ -261,14 +263,6 @@ ValidationResult<RestRequest> withEnabledAuditApi(final RestRequest request) {
         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() {

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java
@@ -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;
@@ -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;
 
@@ -43,7 +41,7 @@ public class SecurityConfigApiAction extends AbstractApiAction {
 
     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();
 
@@ -72,10 +70,13 @@ protected CType getConfigType() {
         return CType.CONFIG;
     }
 
+    @Override
+    protected void consumeParameters(RestRequest request) {}
+
     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))
         )
             .onChangeRequest(Method.PATCH, request -> withAllowedEndpoint(request).map(this::processPatchRequest))
             .override(Method.DELETE, methodNotImplementedHandler)
@@ -89,14 +90,6 @@ ValidationResult<RestRequest> withAllowedEndpoint(final RestRequest request) {
         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() {

diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionValidationTest.java
@@ -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;
@@ -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(

diff --git a/.../dlic/rest/api/SecurityConfigApiTest.java → ...rest/api/SecurityConfigApiActionTest.java b/.../dlic/rest/api/SecurityConfigApiTest.java → ...rest/api/SecurityConfigApiActionTest.java
@@ -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";
     }
 

diff --git a/...est/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionValidationTest.java b/...est/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionValidationTest.java
@@ -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(

diff --git a/.../legacy/LegacySecurityConfigApiTests.java → ...y/LegacySecurityConfigApiActionTests.java b/.../legacy/LegacySecurityConfigApiTests.java → ...y/LegacySecurityConfigApiActionTests.java
@@ -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;