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.

Sign up for GitHub

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

Jump to bottom

Clean up REST API (Part 1) #2900

Merged
merged 2 commits into from
Jul 27, 2023
Merged

Clean up REST API (Part 1) #2900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ef9bcc
REST API code cleaning remove classes
willyborankin Jul 8, 2023
a336e88
REST API code cleaning
willyborankin Jul 8, 2023
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
105 changes: 24 additions & 81 deletions src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java
View file
Open in desktop
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 @@
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 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do handleDelete and handleGet use the JsonNode parameter anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not, I intentionally left them as is, the next PR will change this logic

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 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 @@

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 @@
return;
}

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

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

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java#L225 

Added line #L225 was not covered by tests
}

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

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 @@
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 @@

// 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 @@
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 @@

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 boolean checkConfigUpdateResponse(final ConfigUpdateResponse response) {
stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved

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 @@
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);

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

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java#L491 

Added line #L491 was not covered by tests
}
}

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 @@
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
Open in desktop
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 @@

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

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

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java#L164 

Added line #L164 was not covered by tests

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

Expand Down Expand Up @@ -241,9 +248,29 @@
}

@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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much cleaner than now. Ultimately, I'd like to see this defined with an OpenAPI spec like syntax one day

return ImmutableMap.of("hash", DataType.STRING, "password", DataType.STRING, "current_password", DataType.STRING);
stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved
}
});
}

@Override
Expand Down
Loading
Loading