Misc changes:

- Moved resourceName to EndpointValidator - Added SecurityConfigurationTest Signed-off-by: Andrey Pleskach <ples@aiven.io>
opensearch-project · Aug 19, 2023 · 1c75d36 · 1c75d36
1 parent dedbd64
commit 1c75d36
Show file tree

Hide file tree

Showing 22 changed files with 148 additions and 137 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
@@ -72,6 +72,7 @@
 
 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.conflict;
 import static org.opensearch.security.dlic.rest.api.Responses.forbidden;
 import static org.opensearch.security.dlic.rest.api.Responses.forbiddenMessage;
 import static org.opensearch.security.dlic.rest.api.Responses.internalSeverError;
@@ -83,9 +84,9 @@ public abstract class AbstractApiAction extends BaseRestHandler {
 
     private final static Logger LOGGER = LogManager.getLogger(AbstractApiAction.class);
 
-    private final static Set<String> PATCH_OPERATIONS = Set.of("add", "replace", "remove");
+    private final static Set<String> supportedPatchOperations = Set.of("add", "replace", "remove");
 
-    private final static String PATCH_OPERATIONS_AS_STRING = String.join(",", PATCH_OPERATIONS);
+    private final static String supportedPatchOperationsAsString = String.join(",", supportedPatchOperations);
 
     protected final ConfigurationRepository cl;
     protected final ClusterService cs;
@@ -154,7 +155,8 @@ private void buildDefaultRequestHandlers(final RequestHandler.RequestHandlersBui
     }
 
     protected final ValidationResult<SecurityConfiguration> processDeleteRequest(final RestRequest request) throws IOException {
-        return withRequiredResourceName(request).map(entityName -> loadConfiguration(entityName, false))
+        return endpointValidator.withRequiredEntityName(nameParam(request))
+            .map(entityName -> loadConfiguration(entityName, false))
             .map(endpointValidator::onConfigDelete)
             .map(this::removeEntityFromConfig);
     }
@@ -195,11 +197,11 @@ protected final ValidationResult<JsonNode> withPatchRequestContent(final RestReq
                 return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Wrong request body"));
             }
             for (final var patchOperation : operations) {
-                if (!PATCH_OPERATIONS.contains(patchOperation)) {
+                if (!supportedPatchOperations.contains(patchOperation)) {
                     return ValidationResult.error(
                         RestStatus.BAD_REQUEST,
                         badRequestMessage(
-                            "Unsupported patch operation: " + patchOperation + ". Supported are: " + PATCH_OPERATIONS_AS_STRING
+                            "Unsupported patch operation: " + patchOperation + ". Supported are: " + supportedPatchOperationsAsString
                         )
                     );
                 }
@@ -335,9 +337,10 @@ protected final Set<String> patchEntityNames(final JsonNode patchRequestContent)
     }
 
     protected final ValidationResult<SecurityConfiguration> processPutRequest(final RestRequest request) throws IOException {
-        return withRequiredResourceName(request).map(
-            entityName -> loadConfigurationWithRequestContent(entityName, request, endpointValidator.createRequestContentValidator())
-        ).map(endpointValidator::onConfigChange).map(this::addEntityToConfig);
+        return endpointValidator.withRequiredEntityName(nameParam(request))
+            .map(entityName -> loadConfigurationWithRequestContent(entityName, request, endpointValidator.createRequestContentValidator()))
+            .map(endpointValidator::onConfigChange)
+            .map(this::addEntityToConfig);
     }
 
     protected final ValidationResult<SecurityConfiguration> addEntityToConfig(final SecurityConfiguration securityConfiguration)
@@ -364,14 +367,6 @@ protected final String nameParam(final RestRequest request) {
         return name;
     }
 
-    protected final ValidationResult<String> withRequiredResourceName(final RestRequest request) {
-        final var resourceName = nameParam(request);
-        if (resourceName == null) {
-            return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("No " + getResourceName() + " specified."));
-        }
-        return ValidationResult.success(resourceName);
-    }
-
     protected final ValidationResult<SecurityConfiguration> loadConfigurationWithRequestContent(
         final String entityName,
         final RestRequest request,
@@ -448,7 +443,7 @@ public RequestContentValidator createRequestContentValidator(final Object... par
         };
     }
 
-    protected abstract String getResourceName();
+    // protected abstract String getResourceName();
 
     protected abstract CType getConfigType();
 
@@ -475,9 +470,9 @@ public OnSucessActionListener(RestChannel channel) {
         @Override
         public final void onFailure(Exception e) {
             if (ExceptionsHelper.unwrapCause(e) instanceof VersionConflictEngineException) {
-                Responses.conflict(channel, e.getMessage());
+                conflict(channel, e.getMessage());
             } else {
-                Responses.internalSeverError(channel, "Error " + e.getMessage());
+                internalSeverError(channel, "Error " + e.getMessage());
             }
         }
 

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java
@@ -48,6 +48,7 @@
 
 import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage;
 import static org.opensearch.security.dlic.rest.api.Responses.ok;
+import static org.opensearch.security.dlic.rest.api.Responses.response;
 import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
 import static org.opensearch.security.dlic.rest.support.Utils.hash;
 
@@ -87,11 +88,6 @@ public List<Route> routes() {
         return routes;
     }
 
-    @Override
-    protected String getResourceName() {
-        return RESOURCE_NAME;
-    }
-
     @Override
     protected Endpoint getEndpoint() {
         return Endpoint.ACCOUNT;
@@ -117,7 +113,7 @@ private void accountApiRequestHandlers(RequestHandler.RequestHandlersBuilder req
                     final var remoteAddress = userRemoteAddressAndConfig.getMiddle();
                     final var configuration = userRemoteAddressAndConfig.getRight();
                     userAccount(channel, user, remoteAddress, configuration);
-                }).error((status, toXContent) -> Responses.response(channel, status, toXContent))
+                }).error((status, toXContent) -> response(channel, status, toXContent))
             )
             .onChangeRequest(
                 Method.PUT,

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
@@ -46,8 +46,6 @@
 
 public class ActionGroupsApiAction extends AbstractApiAction {
 
-    protected final static String RESOURCE_NAME = "actiongroup";
-
     private static final List<Route> routes = addRoutesPrefix(
         ImmutableList.of(
             // legacy mapping for backwards compatibility
@@ -94,11 +92,6 @@ protected CType getConfigType() {
         return CType.ACTIONGROUPS;
     }
 
-    @Override
-    protected String getResourceName() {
-        return RESOURCE_NAME;
-    }
-
     @Override
     protected Endpoint getEndpoint() {
         return Endpoint.ACTIONGROUPS;
@@ -113,7 +106,7 @@ protected EndpointValidator createEndpointValidator() {
         return new EndpointValidator() {
             @Override
             public String resourceName() {
-                return RESOURCE_NAME;
+                return "actiongroup";
             }
 
             @Override

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AllowlistApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AllowlistApiAction.java
@@ -77,14 +77,15 @@
  * <p>
  */
 public class AllowlistApiAction extends AbstractApiAction {
+
+    private static final String RESOURCE_NAME = "config";
+
     private static final List<Route> routes = ImmutableList.of(
         new Route(RestRequest.Method.GET, "/_plugins/_security/api/allowlist"),
         new Route(RestRequest.Method.PUT, "/_plugins/_security/api/allowlist"),
         new Route(RestRequest.Method.PATCH, "/_plugins/_security/api/allowlist")
     );
 
-    private static final String RESOURCE_NAME = "config";
-
     @Inject
     public AllowlistApiAction(
         final Settings settings,
@@ -120,11 +121,6 @@ public List<Route> routes() {
         return routes;
     }
 
-    @Override
-    protected String getResourceName() {
-        return RESOURCE_NAME;
-    }
-
     @Override
     protected CType getConfigType() {
         return CType.ALLOWLIST;

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
@@ -231,11 +231,6 @@ public List<Route> routes() {
         return routes;
     }
 
-    @Override
-    protected String getResourceName() {
-        return RESOURCE_NAME;
-    }
-
     @Override
     protected Endpoint getEndpoint() {
         return Endpoint.AUDIT;
@@ -284,7 +279,7 @@ protected EndpointValidator createEndpointValidator() {
 
             @Override
             public String resourceName() {
-                return getResourceName();
+                return RESOURCE_NAME;
             }
 
             @Override
@@ -307,7 +302,7 @@ private ValidationResult<SecurityConfiguration> verifyNotReadonlyFieldUpdated(
             ) {
                 if (!restApiAdminPrivilegesEvaluator().isCurrentUserAdminFor(endpoint())) {
                     final var existingResource = Utils.convertJsonToJackson(securityConfiguration.configuration(), false)
-                        .get(getResourceName());
+                        .get(RESOURCE_NAME);
                     final var targetResource = securityConfiguration.requestContent();
                     if (readonlyFields.stream().anyMatch(path -> !existingResource.at(path).equals(targetResource.at(path)))) {
                         return ValidationResult.error(RestStatus.CONFLICT, conflictMessage("Attempted to update read-only property."));

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AuthTokenProcessorAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AuthTokenProcessorAction.java
@@ -56,11 +56,6 @@ public List<Route> routes() {
         return routes;
     }
 
-    @Override
-    protected String getResourceName() {
-        return "authtoken";
-    }
-
     @Override
     protected CType getConfigType() {
         return null;

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java
@@ -109,12 +109,6 @@ public void onFailure(final Exception e) {
         });
     }
 
-    @Override
-    protected String getResourceName() {
-        // not needed
-        return null;
-    }
-
     @Override
     protected CType getConfigType() {
         return null;

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
@@ -48,6 +48,7 @@
 import static org.opensearch.security.dlic.rest.api.Responses.methodNotImplementedMessage;
 import static org.opensearch.security.dlic.rest.api.Responses.ok;
 import static org.opensearch.security.dlic.rest.api.Responses.payload;
+import static org.opensearch.security.dlic.rest.api.Responses.response;
 import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
 import static org.opensearch.security.dlic.rest.support.Utils.hash;
 
@@ -108,11 +109,6 @@ protected Endpoint getEndpoint() {
         return Endpoint.INTERNALUSERS;
     }
 
-    @Override
-    protected String getResourceName() {
-        return RESOURCE_NAME;
-    }
-
     @Override
     protected CType getConfigType() {
         return CType.INTERNALUSERS;
@@ -130,14 +126,19 @@ private void internalUsersApiRequestHandlers(RequestHandler.RequestHandlersBuild
                 )
                     .map(endpointValidator::entityExists)
                     .valid(securityConfiguration -> generateAuthToken(channel, securityConfiguration))
-                    .error((status, toXContent) -> Responses.response(channel, status, toXContent))
+                    .error((status, toXContent) -> response(channel, status, toXContent))
             )
             .onChangeRequest(Method.PATCH, this::processPatchRequest)
             .onChangeRequest(
                 Method.PUT,
-                request -> withRequiredResourceName(request).map(
-                    username -> loadConfigurationWithRequestContent(username, request, endpointValidator.createRequestContentValidator())
-                )
+                request -> endpointValidator.withRequiredEntityName(nameParam(request))
+                    .map(
+                        username -> loadConfigurationWithRequestContent(
+                            username,
+                            request,
+                            endpointValidator.createRequestContentValidator()
+                        )
+                    )
                     .map(endpointValidator::hasRightsToChangeEntity)
                     .map(this::validateSecurityRoles)
                     .map(securityConfiguration -> createOrUpdateAccount(request, securityConfiguration))
@@ -147,7 +148,7 @@ private void internalUsersApiRequestHandlers(RequestHandler.RequestHandlersBuild
     }
 
     private ValidationResult<String> withAuthTokenPath(final RestRequest request) throws IOException {
-        return withRequiredResourceName(request).map(username -> {
+        return endpointValidator.withRequiredEntityName(nameParam(request)).map(username -> {
             // Handle auth token fetching
             if (!(request.uri().contains("/internalusers/" + username + "/authtoken") && request.uri().endsWith("/authtoken"))) {
                 return ValidationResult.error(RestStatus.NOT_IMPLEMENTED, methodNotImplementedMessage(request.method()));

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java
@@ -102,12 +102,6 @@ protected Endpoint getEndpoint() {
         return Endpoint.MIGRATE;
     }
 
-    @Override
-    protected String getResourceName() {
-        // not needed
-        return null;
-    }
-
     @Override
     protected CType getConfigType() {
         return null;

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/MultiTenancyConfigApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/MultiTenancyConfigApiAction.java
@@ -47,6 +47,7 @@
 import static org.opensearch.rest.RestRequest.Method.GET;
 import static org.opensearch.rest.RestRequest.Method.PUT;
 import static org.opensearch.security.dlic.rest.api.Responses.ok;
+import static org.opensearch.security.dlic.rest.api.Responses.response;
 import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
 
 public class MultiTenancyConfigApiAction extends AbstractApiAction {
@@ -95,11 +96,6 @@ protected Endpoint getEndpoint() {
         return Endpoint.TENANTS;
     }
 
-    @Override
-    protected String getResourceName() {
-        return null;
-    }
-
     @Override
     protected CType getConfigType() {
         return CType.CONFIG;
@@ -165,11 +161,11 @@ private void multiTenancyConfigApiRequestHandlers(RequestHandler.RequestHandlers
             .override(GET, (channel, request, client) -> loadConfiguration(getConfigType(), false, false).valid(configuration -> {
                 final var config = (ConfigV7) configuration.getCEntry(CType.CONFIG.toLCString());
                 ok(channel, multitenancyContent(config));
-            }).error((status, toXContent) -> Responses.response(channel, status, toXContent)))
+            }).error((status, toXContent) -> response(channel, status, toXContent)))
             .override(PUT, (channel, request, client) -> {
                 loadConfigurationWithRequestContent("config", request, createEndpointValidator().createRequestContentValidator()).valid(
                     securityConfiguration -> updateMultitenancy(channel, client, securityConfiguration)
-                ).error((status, toXContent) -> Responses.response(channel, status, toXContent));
+                ).error((status, toXContent) -> response(channel, status, toXContent));
             });
     }
 

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/NodesDnApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/NodesDnApiAction.java
@@ -60,6 +60,9 @@
  * See {@link NodesDnApiTest} for usage examples.
  */
 public class NodesDnApiAction extends AbstractApiAction {
+
+    public static final String RESOURCE_NAME = "nodesdn";
+
     public static final String STATIC_OPENSEARCH_YML_NODES_DN = "STATIC_OPENSEARCH_YML_NODES_DN";
     private final List<String> staticNodesDnFromEsYml;
 
@@ -104,11 +107,6 @@ protected Endpoint getEndpoint() {
         return Endpoint.NODESDN;
     }
 
-    @Override
-    protected String getResourceName() {
-        return "nodesdn";
-    }
-
     @Override
     protected CType getConfigType() {
         return CType.NODESDN;
@@ -145,7 +143,7 @@ protected EndpointValidator createEndpointValidator() {
         return new EndpointValidator() {
             @Override
             public String resourceName() {
-                return getResourceName();
+                return RESOURCE_NAME;
             }
 
             @Override

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
@@ -124,11 +124,6 @@ public List<Route> routes() {
         return routes;
     }
 
-    @Override
-    protected String getResourceName() {
-        return RESOURCE_NAME;
-    }
-
     @Override
     protected CType getConfigType() {
         return CType.ROLES;