Skip to content

Commit 5936137

Browse files
willyborankinwillyborankin
committed
REST API code cleaning (Part 1)
Request content validator code cleaning Signed-off-by: Andrey Pleskach <ples@aiven.io>
1 parent d871af3 commit 5936137

32 files changed

+1369
-385
lines changed

‎src/main/java/org/opensearch/security/DefaultObjectMapper.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import com.fasterxml.jackson.databind.exc.InvalidFormatException;
4545
import com.fasterxml.jackson.databind.exc.MismatchedInputException;
4646
import com.fasterxml.jackson.databind.introspect.BeanPropertyDefinition;
47+
import com.fasterxml.jackson.databind.node.ObjectNode;
4748
import com.fasterxml.jackson.databind.type.TypeFactory;
4849
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
4950
import com.google.common.collect.ImmutableSet;
@@ -103,6 +104,10 @@ public static <T> T getOrDefault(Map<String, Object> properties, String key, T d
103104
return value != null ? value : defaultValue;
104105
}
105106

107+
public static ObjectNode createObjectNode() {
108+
return objectMapper.createObjectNode();
109+
}
110+
106111
@SuppressWarnings("removal")
107112
public static <T> T readTree(JsonNode node, Class<T> clazz) throws IOException {
108113

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

Lines changed: 25 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
import java.io.IOException;
1515
import java.nio.file.Path;
16-
import java.util.Arrays;
1716
import java.util.Collections;
1817
import java.util.Objects;
1918

@@ -30,7 +29,6 @@
3029
import org.opensearch.client.Client;
3130
import org.opensearch.client.node.NodeClient;
3231
import org.opensearch.cluster.service.ClusterService;
33-
import org.opensearch.common.bytes.BytesReference;
3432
import org.opensearch.common.settings.Settings;
3533
import org.opensearch.common.util.concurrent.ThreadContext.StoredContext;
3634
import org.opensearch.common.xcontent.XContentHelper;
@@ -47,14 +45,13 @@
4745
import org.opensearch.rest.RestStatus;
4846
import org.opensearch.security.DefaultObjectMapper;
4947
import org.opensearch.security.action.configupdate.ConfigUpdateAction;
50-
import org.opensearch.security.action.configupdate.ConfigUpdateNodeResponse;
5148
import org.opensearch.security.action.configupdate.ConfigUpdateRequest;
5249
import org.opensearch.security.action.configupdate.ConfigUpdateResponse;
5350
import org.opensearch.security.auditlog.AuditLog;
5451
import org.opensearch.security.configuration.AdminDNs;
5552
import org.opensearch.security.configuration.ConfigurationRepository;
56-
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator;
57-
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator.ErrorType;
53+
import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
54+
import org.opensearch.security.dlic.rest.validation.ValidationError;
5855
import org.opensearch.security.privileges.PrivilegesEvaluator;
5956
import org.opensearch.security.securityconf.DynamicConfigFactory;
6057
import org.opensearch.security.securityconf.Hideable;
@@ -70,7 +67,7 @@
7067

7168
public abstract class AbstractApiAction extends BaseRestHandler {
7269

73-
protected final Logger log = LogManager.getLogger(this.getClass());
70+
private final static Logger LOGGER = LogManager.getLogger(AbstractApiAction.class);
7471

7572
protected final ConfigurationRepository cl;
7673
protected final ClusterService cs;
@@ -121,7 +118,7 @@ protected AbstractApiAction(
121118
this.auditLog = auditLog;
122119
}
123120

124-
protected abstract AbstractConfigurationValidator getValidator(RestRequest request, BytesReference ref, Object... params);
121+
protected abstract RequestContentValidator createValidator(final Object... params);
125122

126123
protected abstract String getResourceName();
127124

@@ -130,25 +127,22 @@ protected AbstractApiAction(
130127
protected void handleApiRequest(final RestChannel channel, final RestRequest request, final Client client) throws IOException {
131128

132129
try {
133-
// validate additional settings, if any
134-
AbstractConfigurationValidator validator = getValidator(request, request.content());
135-
if (!validator.validate()) {
136-
request.params().clear();
137-
badRequestResponse(channel, validator);
138-
return;
139-
}
140130
switch (request.method()) {
141131
case DELETE:
142-
handleDelete(channel, request, client, validator.getContentAsNode());
132+
handleDelete(channel, request, client, null);
143133
break;
144134
case POST:
145-
handlePost(channel, request, client, validator.getContentAsNode());
135+
createValidator().validate(request)
136+
.valid(jsonContent -> handlePost(channel, request, client, jsonContent))
137+
.error(toXContent -> requestContentInvalid(request, channel, toXContent));
146138
break;
147139
case PUT:
148-
handlePut(channel, request, client, validator.getContentAsNode());
140+
createValidator().validate(request)
141+
.valid(jsonContent -> handlePut(channel, request, client, jsonContent))
142+
.error(toXContent -> requestContentInvalid(request, channel, toXContent));
149143
break;
150144
case GET:
151-
handleGet(channel, request, client, validator.getContentAsNode());
145+
handleGet(channel, request, client, null);
152146
break;
153147
default:
154148
throw new IllegalArgumentException(request.method() + " not supported");
@@ -162,6 +156,11 @@ protected void handleApiRequest(final RestChannel channel, final RestRequest req
162156
}
163157
}
164158

159+
protected void requestContentInvalid(final RestRequest request, final RestChannel channel, final ToXContent toXContent) {
160+
request.params().clear();
161+
badRequestResponse(channel, toXContent);
162+
}
163+
165164
protected void handleDelete(final RestChannel channel, final RestRequest request, final Client client, final JsonNode content)
166165
throws IOException {
167166
final String name = request.param("name");
@@ -202,16 +201,12 @@ public void onResponse(IndexResponse response) {
202201

203202
protected void handlePut(final RestChannel channel, final RestRequest request, final Client client, final JsonNode content)
204203
throws IOException {
205-
206204
final String name = request.param("name");
207-
208205
if (name == null || name.length() == 0) {
209206
badRequestResponse(channel, "No " + getResourceName() + " specified.");
210207
return;
211208
}
212-
213209
final SecurityDynamicConfiguration<?> existingConfiguration = load(getConfigName(), false);
214-
215210
if (existingConfiguration.getSeqNo() < 0) {
216211
forbidden(
217212
channel,
@@ -229,8 +224,8 @@ protected void handlePut(final RestChannel channel, final RestRequest request, f
229224
return;
230225
}
231226

232-
if (log.isTraceEnabled() && content != null) {
233-
log.trace(content.toString());
227+
if (LOGGER.isTraceEnabled() && content != null) {
228+
LOGGER.trace(content.toString());
234229
}
235230

236231
boolean existed = existingConfiguration.exists(name);
@@ -276,28 +271,21 @@ protected boolean hasPermissionsToCreate(
276271
}
277272

278273
protected void handleGet(final RestChannel channel, RestRequest request, Client client, final JsonNode content) throws IOException {
279-
280274
final String resourcename = request.param("name");
281-
282275
final SecurityDynamicConfiguration<?> configuration = load(getConfigName(), true);
283276
filter(configuration);
284-
285277
// no specific resource requested, return complete config
286278
if (resourcename == null || resourcename.length() == 0) {
287279

288280
successResponse(channel, configuration);
289281
return;
290282
}
291-
292283
if (!configuration.exists(resourcename)) {
293284
notFound(channel, "Resource '" + resourcename + "' not found.");
294285
return;
295286
}
296-
297287
configuration.removeOthers(resourcename);
298288
successResponse(channel, configuration);
299-
300-
return;
301289
}
302290

303291
protected final SecurityDynamicConfiguration<?> load(final CType config, boolean logComplianceEvent) {
@@ -307,15 +295,6 @@ protected final SecurityDynamicConfiguration<?> load(final CType config, boolean
307295
return DynamicConfigFactory.addStatics(loaded);
308296
}
309297

310-
protected final SecurityDynamicConfiguration<?> load(final CType config, boolean logComplianceEvent, boolean acceptInvalid) {
311-
SecurityDynamicConfiguration<?> loaded = cl.getConfigurationsFromIndex(
312-
Collections.singleton(config),
313-
logComplianceEvent,
314-
acceptInvalid
315-
).get(config).deepClone();
316-
return DynamicConfigFactory.addStatics(loaded);
317-
}
318-
319298
protected boolean ensureIndexExists() {
320299
if (!cs.state().metadata().hasConcreteIndex(this.securityIndexName)) {
321300
return false;
@@ -436,7 +415,7 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
436415

437416
// check if .opendistro_security index has been initialized
438417
if (!ensureIndexExists()) {
439-
return channel -> internalErrorResponse(channel, ErrorType.SECURITY_NOT_INITIALIZED.getMessage());
418+
return channel -> internalErrorResponse(channel, ValidationError.SECURITY_NOT_INITIALIZED.message());
440419
}
441420

442421
// check if request is authorized
@@ -445,7 +424,7 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
445424
final User user = (User) threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
446425
final String userName = user == null ? null : user.getName();
447426
if (authError != null) {
448-
log.error("No permission to access REST API: " + authError);
427+
LOGGER.error("No permission to access REST API: " + authError);
449428
auditLog.logMissingPrivileges(authError, userName, request);
450429
// for rest request
451430
request.params().clear();
@@ -467,7 +446,7 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
467446

468447
handleApiRequest(channel, request, client);
469448
} catch (Exception e) {
470-
log.error("Error processing request {}", request, e);
449+
LOGGER.error("Error processing request {}", request, e);
471450
try {
472451
channel.sendResponse(new BytesRestResponse(channel, e));
473452
} catch (IOException ioe) {
@@ -477,37 +456,6 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
477456
});
478457
}
479458

480-
protected boolean checkConfigUpdateResponse(final ConfigUpdateResponse response) {
481-
482-
final int nodeCount = cs.state().getNodes().getNodes().size();
483-
final int expectedConfigCount = 1;
484-
485-
boolean success = response.getNodes().size() == nodeCount;
486-
if (!success) {
487-
log.error("Expected " + nodeCount + " nodes to return response, but got only " + response.getNodes().size());
488-
}
489-
490-
for (final String nodeId : response.getNodesMap().keySet()) {
491-
final ConfigUpdateNodeResponse node = response.getNodesMap().get(nodeId);
492-
final boolean successNode = node.getUpdatedConfigTypes() != null && node.getUpdatedConfigTypes().length == expectedConfigCount;
493-
494-
if (!successNode) {
495-
log.error(
496-
"Expected "
497-
+ expectedConfigCount
498-
+ " config types for node "
499-
+ nodeId
500-
+ " but got only "
501-
+ Arrays.toString(node.getUpdatedConfigTypes())
502-
);
503-
}
504-
505-
success = success && successNode;
506-
}
507-
508-
return success;
509-
}
510-
511459
protected static XContentBuilder convertToJson(RestChannel channel, ToXContent toxContent) {
512460
try {
513461
XContentBuilder builder = channel.newBuilder();
@@ -543,12 +491,12 @@ protected void successResponse(RestChannel channel) {
543491
channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder));
544492
} catch (IOException e) {
545493
internalErrorResponse(channel, "Unable to fetch license: " + e.getMessage());
546-
log.error("Cannot fetch convert license to XContent due to", e);
494+
LOGGER.error("Cannot fetch convert license to XContent due to", e);
547495
}
548496
}
549497

550-
protected void badRequestResponse(RestChannel channel, AbstractConfigurationValidator validator) {
551-
channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, validator.errorsAsXContent(channel)));
498+
protected void badRequestResponse(RestChannel channel, ToXContent validationResult) {
499+
channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, convertToJson(channel, validationResult)));
552500
}
553501

554502
protected void successResponse(RestChannel channel, String message) {
@@ -575,10 +523,6 @@ protected void internalErrorResponse(RestChannel channel, String message) {
575523
response(channel, RestStatus.INTERNAL_SERVER_ERROR, message);
576524
}
577525

578-
protected void unprocessable(RestChannel channel, String message) {
579-
response(channel, RestStatus.UNPROCESSABLE_ENTITY, message);
580-
}
581-
582526
protected void conflict(RestChannel channel, String message) {
583527
response(channel, RestStatus.CONFLICT, message);
584528
}

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

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,20 @@
1414
import java.io.IOException;
1515
import java.nio.file.Path;
1616
import java.util.List;
17+
import java.util.Map;
1718
import java.util.Set;
1819

1920
import com.fasterxml.jackson.databind.JsonNode;
2021
import com.google.common.collect.ImmutableList;
22+
import com.google.common.collect.ImmutableMap;
23+
import com.google.common.collect.ImmutableSet;
24+
import org.apache.logging.log4j.LogManager;
25+
import org.apache.logging.log4j.Logger;
2126
import org.bouncycastle.crypto.generators.OpenBSDBCrypt;
2227

2328
import org.opensearch.action.index.IndexResponse;
2429
import org.opensearch.client.Client;
2530
import org.opensearch.cluster.service.ClusterService;
26-
import org.opensearch.common.bytes.BytesReference;
2731
import org.opensearch.common.settings.Settings;
2832
import org.opensearch.common.transport.TransportAddress;
2933
import org.opensearch.common.util.concurrent.ThreadContext;
@@ -38,8 +42,8 @@
3842
import org.opensearch.security.auditlog.AuditLog;
3943
import org.opensearch.security.configuration.AdminDNs;
4044
import org.opensearch.security.configuration.ConfigurationRepository;
41-
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator;
42-
import org.opensearch.security.dlic.rest.validation.AccountValidator;
45+
import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
46+
import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType;
4347
import org.opensearch.security.privileges.PrivilegesEvaluator;
4448
import org.opensearch.security.securityconf.Hashed;
4549
import org.opensearch.security.securityconf.impl.CType;
@@ -58,6 +62,9 @@
5862
* Currently this action serves GET and PUT request for /_opendistro/_security/api/account endpoint
5963
*/
6064
public class AccountApiAction extends AbstractApiAction {
65+
66+
private final static Logger LOGGER = LogManager.getLogger(AccountApiAction.class);
67+
6168
private static final String RESOURCE_NAME = "account";
6269
private static final List<Route> routes = addRoutesPrefix(
6370
ImmutableList.of(new Route(Method.GET, "/account"), new Route(Method.PUT, "/account"))
@@ -154,7 +161,7 @@ protected void handleGet(RestChannel channel, RestRequest request, Client client
154161

155162
response = new BytesRestResponse(RestStatus.OK, builder);
156163
} catch (final Exception exception) {
157-
log.error(exception.toString());
164+
LOGGER.error(exception.toString());
158165

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

@@ -241,9 +248,29 @@ public void onResponse(IndexResponse response) {
241248
}
242249

243250
@Override
244-
protected AbstractConfigurationValidator getValidator(RestRequest request, BytesReference ref, Object... params) {
251+
protected RequestContentValidator createValidator(final Object... params) {
245252
final User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
246-
return new AccountValidator(request, ref, this.settings, user.getName());
253+
return RequestContentValidator.of(new RequestContentValidator.ValidationContext() {
254+
@Override
255+
public Object[] params() {
256+
return new Object[] { user.getName() };
257+
}
258+
259+
@Override
260+
public Settings settings() {
261+
return settings;
262+
}
263+
264+
@Override
265+
public Set<String> mandatoryKeys() {
266+
return ImmutableSet.of("current_password");
267+
}
268+
269+
@Override
270+
public Map<String, RequestContentValidator.DataType> allowedKeys() {
271+
return ImmutableMap.of("hash", DataType.STRING, "password", DataType.STRING, "current_password", DataType.STRING);
272+
}
273+
});
247274
}
248275

249276
@Override

0 commit comments

Comments
 (0)