Skip to content

Commit 80d3e61

Browse files
willyborankinwillyborankin
committed
REST API code cleaning
Request content validator code cleaning Signed-off-by: Andrey Pleskach <ples@aiven.io>
1 parent 8abdd15 commit 80d3e61

30 files changed

+1405
-352
lines changed

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

Lines changed: 24 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,12 @@
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;
5854
import org.opensearch.security.privileges.PrivilegesEvaluator;
5955
import org.opensearch.security.securityconf.DynamicConfigFactory;
6056
import org.opensearch.security.securityconf.impl.CType;
@@ -68,7 +64,7 @@
6864

6965
public abstract class AbstractApiAction extends BaseRestHandler {
7066

71-
protected final Logger log = LogManager.getLogger(this.getClass());
67+
private final static Logger LOGGER = LogManager.getLogger(AbstractApiAction.class);
7268

7369
protected final ConfigurationRepository cl;
7470
protected final ClusterService cs;
@@ -119,7 +115,7 @@ protected AbstractApiAction(
119115
this.auditLog = auditLog;
120116
}
121117

122-
protected abstract AbstractConfigurationValidator getValidator(RestRequest request, BytesReference ref, Object... params);
118+
protected abstract RequestContentValidator createValidator(final Object... params);
123119

124120
protected abstract String getResourceName();
125121

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

130126
try {
131-
// validate additional settings, if any
132-
AbstractConfigurationValidator validator = getValidator(request, request.content());
133-
if (!validator.validate()) {
134-
request.params().clear();
135-
badRequestResponse(channel, validator);
136-
return;
137-
}
138127
switch (request.method()) {
139128
case DELETE:
140-
handleDelete(channel, request, client, validator.getContentAsNode());
129+
handleDelete(channel, request, client, null);
141130
break;
142131
case POST:
143-
handlePost(channel, request, client, validator.getContentAsNode());
132+
createValidator().validate(request)
133+
.valid(jsonContent -> handlePost(channel, request, client, jsonContent))
134+
.error(toXContent -> requestContentInvalid(request, channel, toXContent));
144135
break;
145136
case PUT:
146-
handlePut(channel, request, client, validator.getContentAsNode());
137+
createValidator().validate(request)
138+
.valid(jsonContent -> handlePut(channel, request, client, jsonContent))
139+
.error(toXContent -> requestContentInvalid(request, channel, toXContent));
147140
break;
148141
case GET:
149-
handleGet(channel, request, client, validator.getContentAsNode());
142+
handleGet(channel, request, client, null);
150143
break;
151144
default:
152145
throw new IllegalArgumentException(request.method() + " not supported");
@@ -160,6 +153,11 @@ protected void handleApiRequest(final RestChannel channel, final RestRequest req
160153
}
161154
}
162155

156+
protected void requestContentInvalid(final RestRequest request, final RestChannel channel, final ToXContent toXContent) {
157+
request.params().clear();
158+
badRequestResponse(channel, toXContent);
159+
}
160+
163161
protected void handleDelete(final RestChannel channel, final RestRequest request, final Client client, final JsonNode content)
164162
throws IOException {
165163
final String name = request.param("name");
@@ -200,16 +198,12 @@ public void onResponse(IndexResponse response) {
200198

201199
protected void handlePut(final RestChannel channel, final RestRequest request, final Client client, final JsonNode content)
202200
throws IOException {
203-
204201
final String name = request.param("name");
205-
206202
if (name == null || name.length() == 0) {
207203
badRequestResponse(channel, "No " + getResourceName() + " specified.");
208204
return;
209205
}
210-
211206
final SecurityDynamicConfiguration<?> existingConfiguration = load(getConfigName(), false);
212-
213207
if (existingConfiguration.getSeqNo() < 0) {
214208
forbidden(
215209
channel,
@@ -227,8 +221,8 @@ protected void handlePut(final RestChannel channel, final RestRequest request, f
227221
return;
228222
}
229223

230-
if (log.isTraceEnabled() && content != null) {
231-
log.trace(content.toString());
224+
if (LOGGER.isTraceEnabled() && content != null) {
225+
LOGGER.trace(content.toString());
232226
}
233227

234228
boolean existed = existingConfiguration.exists(name);
@@ -274,28 +268,21 @@ protected boolean hasPermissionsToCreate(
274268
}
275269

276270
protected void handleGet(final RestChannel channel, RestRequest request, Client client, final JsonNode content) throws IOException {
277-
278271
final String resourcename = request.param("name");
279-
280272
final SecurityDynamicConfiguration<?> configuration = load(getConfigName(), true);
281273
filter(configuration);
282-
283274
// no specific resource requested, return complete config
284275
if (resourcename == null || resourcename.length() == 0) {
285276

286277
successResponse(channel, configuration);
287278
return;
288279
}
289-
290280
if (!configuration.exists(resourcename)) {
291281
notFound(channel, "Resource '" + resourcename + "' not found.");
292282
return;
293283
}
294-
295284
configuration.removeOthers(resourcename);
296285
successResponse(channel, configuration);
297-
298-
return;
299286
}
300287

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

308-
protected final SecurityDynamicConfiguration<?> load(final CType config, boolean logComplianceEvent, boolean acceptInvalid) {
309-
SecurityDynamicConfiguration<?> loaded = cl.getConfigurationsFromIndex(
310-
Collections.singleton(config),
311-
logComplianceEvent,
312-
acceptInvalid
313-
).get(config).deepClone();
314-
return DynamicConfigFactory.addStatics(loaded);
315-
}
316-
317295
protected boolean ensureIndexExists() {
318296
if (!cs.state().metadata().hasConcreteIndex(this.securityIndexName)) {
319297
return false;
@@ -434,7 +412,7 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
434412

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

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

466444
handleApiRequest(channel, request, client);
467445
} catch (Exception e) {
468-
log.error("Error processing request {}", request, e);
446+
LOGGER.error("Error processing request {}", request, e);
469447
try {
470448
channel.sendResponse(new BytesRestResponse(channel, e));
471449
} catch (IOException ioe) {
@@ -475,37 +453,6 @@ protected final RestChannelConsumer prepareRequest(RestRequest request, NodeClie
475453
});
476454
}
477455

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

548-
protected void badRequestResponse(RestChannel channel, AbstractConfigurationValidator validator) {
549-
channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, validator.errorsAsXContent(channel)));
495+
protected void badRequestResponse(RestChannel channel, ToXContent validationResult) {
496+
channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, convertToJson(channel, validationResult)));
550497
}
551498

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

576-
protected void unprocessable(RestChannel channel, String message) {
577-
response(channel, RestStatus.UNPROCESSABLE_ENTITY, message);
578-
}
579-
580523
protected void conflict(RestChannel channel, String message) {
581524
response(channel, RestStatus.CONFLICT, message);
582525
}

‎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)