Skip to content

Commit

Permalink
Clean up REST API (Part 1) (opensearch-project#2900)
Browse files Browse the repository at this point in the history 
### Description

The aim of this PR is to start cleaning code in REST API since with the
current implementation is difficult to understand and support.

Changes:

- Implemented new `RequestConetnValidator` class which uses the same
validation logic as `AbstractConfigurationValidator`
- Removed all redundant `AbstractConfigurationValidator` extensions  

### Testing
[Please provide details of testing done: unit testing, integration
testing and manual testing]

### Check List
- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Andrey Pleskach <ples@aiven.io>
  • Loading branch information
@willyborankin
willyborankin authored Jul 27, 2023
1 parent 8063e1b commit 6bac470
Show file tree
Hide file tree
Showing 45 changed files with 1,405 additions and 1,307 deletions.
105 changes: 24 additions & 81 deletions src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

import java.io.IOException;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Collections;
import java.util.Objects;

Expand All @@ -30,7 +29,6 @@
import org.opensearch.client.Client;
import org.opensearch.client.node.NodeClient;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.core.common.bytes.BytesReference;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.concurrent.ThreadContext.StoredContext;
import org.opensearch.common.xcontent.XContentHelper;
Expand All @@ -47,14 +45,12 @@
import org.opensearch.core.rest.RestStatus;
import org.opensearch.security.DefaultObjectMapper;
import org.opensearch.security.action.configupdate.ConfigUpdateAction;
import org.opensearch.security.action.configupdate.ConfigUpdateNodeResponse;
import org.opensearch.security.action.configupdate.ConfigUpdateRequest;
import org.opensearch.security.action.configupdate.ConfigUpdateResponse;
import org.opensearch.security.auditlog.AuditLog;
import org.opensearch.security.configuration.AdminDNs;
import org.opensearch.security.configuration.ConfigurationRepository;
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator;
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator.ErrorType;
import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
import org.opensearch.security.privileges.PrivilegesEvaluator;
import org.opensearch.security.securityconf.DynamicConfigFactory;
import org.opensearch.security.securityconf.impl.CType;
Expand All @@ -68,7 +64,7 @@

public abstract class AbstractApiAction extends BaseRestHandler {

protected final Logger log = LogManager.getLogger(this.getClass());
private final static Logger LOGGER = LogManager.getLogger(AbstractApiAction.class);

protected final ConfigurationRepository cl;
protected final ClusterService cs;
Expand Down Expand Up @@ -119,7 +115,7 @@ protected AbstractApiAction(
this.auditLog = auditLog;
}

protected abstract AbstractConfigurationValidator getValidator(RestRequest request, BytesReference ref, Object... params);
protected abstract RequestContentValidator createValidator(final Object... params);

protected abstract String getResourceName();

Expand All @@ -128,25 +124,22 @@ protected AbstractApiAction(
protected void handleApiRequest(final RestChannel channel, final RestRequest request, final Client client) throws IOException {

try {
// validate additional settings, if any
AbstractConfigurationValidator validator = getValidator(request, request.content());
if (!validator.validate()) {
request.params().clear();
badRequestResponse(channel, validator);
return;
}
switch (request.method()) {
case DELETE:
handleDelete(channel, request, client, validator.getContentAsNode());
handleDelete(channel, request, client, null);
break;
case POST:
handlePost(channel, request, client, validator.getContentAsNode());
createValidator().validate(request)
.valid(jsonContent -> handlePost(channel, request, client, jsonContent))
.error(toXContent -> requestContentInvalid(request, channel, toXContent));
break;
case PUT:
handlePut(channel, request, client, validator.getContentAsNode());
createValidator().validate(request)
.valid(jsonContent -> handlePut(channel, request, client, jsonContent))
.error(toXContent -> requestContentInvalid(request, channel, toXContent));
break;
case GET:
handleGet(channel, request, client, validator.getContentAsNode());
handleGet(channel, request, client, null);
break;
default:
throw new IllegalArgumentException(request.method() + " not supported");
Expand All @@ -160,6 +153,11 @@ protected void handleApiRequest(final RestChannel channel, final RestRequest req
}
}

protected void requestContentInvalid(final RestRequest request, final RestChannel channel, final ToXContent toXContent) {
request.params().clear();
badRequestResponse(channel, toXContent);
}

protected void handleDelete(final RestChannel channel, final RestRequest request, final Client client, final JsonNode content)
throws IOException {
final String name = request.param("name");
Expand Down Expand Up @@ -200,16 +198,12 @@ public void onResponse(IndexResponse response) {

protected void handlePut(final RestChannel channel, final RestRequest request, final Client client, final JsonNode content)
throws IOException {

final String name = request.param("name");

if (name == null || name.length() == 0) {
badRequestResponse(channel, "No " + getResourceName() + " specified.");
return;
}

final SecurityDynamicConfiguration<?> existingConfiguration = load(getConfigName(), false);

if (existingConfiguration.getSeqNo() < 0) {
forbidden(
channel,
Expand All @@ -227,8 +221,8 @@ protected void handlePut(final RestChannel channel, final RestRequest request, f
return;
}

if (log.isTraceEnabled() && content != null) {
log.trace(content.toString());
if (LOGGER.isTraceEnabled() && content != null) {
LOGGER.trace(content.toString());
}

boolean existed = existingConfiguration.exists(name);
Expand Down Expand Up @@ -274,28 +268,21 @@ protected boolean hasPermissionsToCreate(
}

protected void handleGet(final RestChannel channel, RestRequest request, Client client, final JsonNode content) throws IOException {

final String resourcename = request.param("name");

final SecurityDynamicConfiguration<?> configuration = load(getConfigName(), true);
filter(configuration);

// no specific resource requested, return complete config
if (resourcename == null || resourcename.length() == 0) {

successResponse(channel, configuration);
return;
}

if (!configuration.exists(resourcename)) {
notFound(channel, "Resource '" + resourcename + "' not found.");
return;
}

configuration.removeOthers(resourcename);
successResponse(channel, configuration);

return;
}

protected final SecurityDynamicConfiguration<?> load(final CType config, boolean logComplianceEvent) {
Expand All @@ -305,15 +292,6 @@ protected final SecurityDynamicConfiguration<?> load(final CType config, boolean
return DynamicConfigFactory.addStatics(loaded);
}

protected final SecurityDynamicConfiguration<?> load(final CType config, boolean logComplianceEvent, boolean acceptInvalid) {
SecurityDynamicConfiguration<?> loaded = cl.getConfigurationsFromIndex(
Collections.singleton(config),
logComplianceEvent,
acceptInvalid
).get(config).deepClone();
return DynamicConfigFactory.addStatics(loaded);
}

protected boolean ensureIndexExists() {
if (!cs.state().metadata().hasConcreteIndex(this.securityIndexName)) {
return false;
Expand Down Expand Up @@ -434,7 +412,7 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie

// check if .opendistro_security index has been initialized
if (!ensureIndexExists()) {
return channel -> internalErrorResponse(channel, ErrorType.SECURITY_NOT_INITIALIZED.getMessage());
return channel -> internalErrorResponse(channel, RequestContentValidator.ValidationError.SECURITY_NOT_INITIALIZED.message());
}

// check if request is authorized
Expand All @@ -443,7 +421,7 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
final User user = (User) threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
final String userName = user == null ? null : user.getName();
if (authError != null) {
log.error("No permission to access REST API: " + authError);
LOGGER.error("No permission to access REST API: " + authError);
auditLog.logMissingPrivileges(authError, userName, request);
// for rest request
request.params().clear();
Expand All @@ -465,7 +443,7 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie

handleApiRequest(channel, request, client);
} catch (Exception e) {
log.error("Error processing request {}", request, e);
LOGGER.error("Error processing request {}", request, e);
try {
channel.sendResponse(new BytesRestResponse(channel, e));
} catch (IOException ioe) {
Expand All @@ -475,37 +453,6 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
});
}

protected boolean checkConfigUpdateResponse(final ConfigUpdateResponse response) {

final int nodeCount = cs.state().getNodes().getNodes().size();
final int expectedConfigCount = 1;

boolean success = response.getNodes().size() == nodeCount;
if (!success) {
log.error("Expected " + nodeCount + " nodes to return response, but got only " + response.getNodes().size());
}

for (final String nodeId : response.getNodesMap().keySet()) {
final ConfigUpdateNodeResponse node = response.getNodesMap().get(nodeId);
final boolean successNode = node.getUpdatedConfigTypes() != null && node.getUpdatedConfigTypes().length == expectedConfigCount;

if (!successNode) {
log.error(
"Expected "
+ expectedConfigCount
+ " config types for node "
+ nodeId
+ " but got only "
+ Arrays.toString(node.getUpdatedConfigTypes())
);
}

success = success && successNode;
}

return success;
}

protected static XContentBuilder convertToJson(RestChannel channel, ToXContent toxContent) {
try {
XContentBuilder builder = channel.newBuilder();
Expand Down Expand Up @@ -541,12 +488,12 @@ protected void successResponse(RestChannel channel) {
channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder));
} catch (IOException e) {
internalErrorResponse(channel, "Unable to fetch license: " + e.getMessage());
log.error("Cannot fetch convert license to XContent due to", e);
LOGGER.error("Cannot fetch convert license to XContent due to", e);
}
}

protected void badRequestResponse(RestChannel channel, AbstractConfigurationValidator validator) {
channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, validator.errorsAsXContent(channel)));
protected void badRequestResponse(RestChannel channel, ToXContent validationResult) {
channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, convertToJson(channel, validationResult)));
}

protected void successResponse(RestChannel channel, String message) {
Expand All @@ -573,10 +520,6 @@ protected void internalErrorResponse(RestChannel channel, String message) {
response(channel, RestStatus.INTERNAL_SERVER_ERROR, message);
}

protected void unprocessable(RestChannel channel, String message) {
response(channel, RestStatus.UNPROCESSABLE_ENTITY, message);
}

protected void conflict(RestChannel channel, String message) {
response(channel, RestStatus.CONFLICT, message);
}
Expand Down
39 changes: 33 additions & 6 deletions src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,20 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.List;
import java.util.Map;
import java.util.Set;

import com.fasterxml.jackson.databind.JsonNode;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.bouncycastle.crypto.generators.OpenBSDBCrypt;

import org.opensearch.action.index.IndexResponse;
import org.opensearch.client.Client;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.core.common.bytes.BytesReference;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.transport.TransportAddress;
import org.opensearch.common.util.concurrent.ThreadContext;
Expand All @@ -38,8 +42,8 @@
import org.opensearch.security.auditlog.AuditLog;
import org.opensearch.security.configuration.AdminDNs;
import org.opensearch.security.configuration.ConfigurationRepository;
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator;
import org.opensearch.security.dlic.rest.validation.AccountValidator;
import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType;
import org.opensearch.security.privileges.PrivilegesEvaluator;
import org.opensearch.security.securityconf.Hashed;
import org.opensearch.security.securityconf.impl.CType;
Expand All @@ -58,6 +62,9 @@
* Currently this action serves GET and PUT request for /_opendistro/_security/api/account endpoint
*/
public class AccountApiAction extends AbstractApiAction {

private final static Logger LOGGER = LogManager.getLogger(AccountApiAction.class);

private static final String RESOURCE_NAME = "account";
private static final List<Route> routes = addRoutesPrefix(
ImmutableList.of(new Route(Method.GET, "/account"), new Route(Method.PUT, "/account"))
Expand Down Expand Up @@ -154,7 +161,7 @@ protected void handleGet(RestChannel channel, RestRequest request, Client client

response = new BytesRestResponse(RestStatus.OK, builder);
} catch (final Exception exception) {
log.error(exception.toString());
LOGGER.error(exception.toString());

builder.startObject().field("error", exception.toString()).endObject();

Expand Down Expand Up @@ -241,9 +248,29 @@ public void onResponse(IndexResponse response) {
}

@Override
protected AbstractConfigurationValidator getValidator(RestRequest request, BytesReference ref, Object... params) {
protected RequestContentValidator createValidator(final Object... params) {
final User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
return new AccountValidator(request, ref, this.settings, user.getName());
return RequestContentValidator.of(new RequestContentValidator.ValidationContext() {
@Override
public Object[] params() {
return new Object[] { user.getName() };
}

@Override
public Settings settings() {
return settings;
}

@Override
public Set<String> mandatoryKeys() {
return ImmutableSet.of("current_password");
}

@Override
public Map<String, RequestContentValidator.DataType> allowedKeys() {
return ImmutableMap.of("hash", DataType.STRING, "password", DataType.STRING, "current_password", DataType.STRING);
}
});
}

@Override
Expand Down
Loading

0 comments on commit 6bac470

Please sign in to comment.