Skip to content

Commit

Permalink
Address comments:
Browse files Browse the repository at this point in the history 
- extract configuration into separate class
- SecurityConfiguration is a companion class now
 - Renamed methods:
    - getResourceName -> getConfigurationName
    - getConfigName -> getConfigurationType
    So each endpoint works with its configuration and change an entity
    in the config

Signed-off-by: Andrey Pleskach <ples@aiven.io>
  • Loading branch information
@willyborankin
willyborankin committed Aug 13, 2023
1 parent ec30ef3 commit 0e2c1f1
Show file tree
Hide file tree
Showing 31 changed files with 937 additions and 482 deletions.
281 changes: 107 additions & 174 deletions src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java
View file

Large diffs are not rendered by default.

40 changes: 27 additions & 13 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 @@ -11,9 +11,12 @@

package org.opensearch.security.dlic.rest.api;

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.commons.lang3.tuple.Triple;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand Down Expand Up @@ -106,7 +109,7 @@ public List<Route> routes() {
}

@Override
protected String getResourceName() {
protected String getConfigurationName() {
return RESOURCE_NAME;
}

Expand All @@ -116,7 +119,7 @@ protected Endpoint getEndpoint() {
}

@Override
protected CType getConfigName() {
protected CType getConfigurationType() {
return CType.INTERNALUSERS;
}

Expand All @@ -126,7 +129,7 @@ private void accountApiRequestHandlers(RequestHandler.RequestHandlersBuilder req
.override(Method.GET, (channel, request, client) ->
withUserAndRemoteAddress().map(
userAndRemoteAddress ->
loadConfiguration(getConfigName(), false)
loadConfiguration(getConfigurationType(), false, false)
.map(configuration ->
ValidationResult.success(
Triple.of(
Expand All @@ -145,9 +148,12 @@ private void accountApiRequestHandlers(RequestHandler.RequestHandlersBuilder req
withUserAndRemoteAddress()
.map(userAndRemoteAddress -> {
final var user = userAndRemoteAddress.getLeft();
return loadSecurityConfigurationWithRequestContent(user.getName(), request)
.map(this::resourceExists)
.map(this::resourceCanBeChanged);
return loadConfigurationWithRequestContent(user.getName(), request)
.map(contentAndConfiguration ->
configurationHasEntity(contentAndConfiguration.getRight())
.map(configurationValidators::hasRightsToChangeImmutableEntity)
.map(ignore -> ValidationResult.success(contentAndConfiguration))
);
}).map(this::passwordCanBeValidated)
.valid(securityConfiguration -> updateUserPassword(channel, client, securityConfiguration))
.error((status, toXContent) -> Responses.response(channel, status, toXContent))
Expand Down Expand Up @@ -178,21 +184,29 @@ private void userAccount(
);
}

private ValidationResult<SecurityConfiguration> passwordCanBeValidated(final SecurityConfiguration securityConfiguration) {
final var username = securityConfiguration.resourceName();
final var content = securityConfiguration.requestContent();
private ValidationResult<Pair<JsonNode, SecurityConfiguration>> passwordCanBeValidated(
final Pair<JsonNode, SecurityConfiguration> contentAndSecurityConfiguration
) {
final var securityConfiguration = contentAndSecurityConfiguration.getRight();
final var username = securityConfiguration.entityName();
final ObjectNode content = (ObjectNode) contentAndSecurityConfiguration.getLeft();
final var currentPassword = content.get("current_password").asText();
final var internalUserEntry = (Hashed) securityConfiguration.configuration().getCEntry(username);
final var currentHash = internalUserEntry.getHash();
if (currentHash == null || !OpenBSDBCrypt.checkPassword(currentHash, currentPassword.toCharArray())) {
return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Could not validate your current password."));
}
return ValidationResult.success(securityConfiguration);
return ValidationResult.success(contentAndSecurityConfiguration);
}

private void updateUserPassword(final RestChannel channel, final Client client, final SecurityConfiguration securityConfiguration) {
final var username = securityConfiguration.resourceName();
final var securityJsonNode = new SecurityJsonNode(securityConfiguration.requestContent());
private void updateUserPassword(
final RestChannel channel,
final Client client,
final Pair<JsonNode, SecurityConfiguration> contentAndSecurityConfiguration
) {
final var securityConfiguration = contentAndSecurityConfiguration.getRight();
final var username = securityConfiguration.entityName();
final var securityJsonNode = new SecurityJsonNode(contentAndSecurityConfiguration.getLeft());
final var internalUserEntry = (Hashed) securityConfiguration.configuration().getCEntry(username);
// if password is set, it takes precedence over hash
final var password = securityJsonNode.get("password").asString();
Expand Down
50 changes: 25 additions & 25 deletions src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,17 +101,25 @@ private void actionGroupsApiRequestHandlers(RequestHandler.RequestHandlersBuilde
processPutRequest(request)
.map(this::actionGroupNameIsNotSameAsRoleName)
.map(this::hasSelfReference)
.map(this::canChangeObjectWithRestAdminPermissions))
.map(configurationValidators::canChangeObjectWithRestAdminPermissions))
.onChangeRequest(Method.DELETE, request ->
processDeleteRequest(request).map(this::canChangeObjectWithRestAdminPermissions))
// process delete by default removed entity after validation.
// in this an additional check needs to be added for REST API
withRequiredEntityName(request)
.map(entityName ->
loadConfiguration(getConfigurationType(), false, false)
.map(configuration -> toSecurityConfiguration(entityName, configuration)))
.map(this::deleteRequestConfigurationValidator)
.map(configurationValidators::canChangeObjectWithRestAdminPermissions)
.map(this::removeEntityFromConfiguration))
.override(Method.POST, methodNotImplementedHandler);
// spotless:on
}

private ValidationResult<SecurityConfiguration> actionGroupNameIsNotSameAsRoleName(final SecurityConfiguration securityConfiguration)
throws IOException {
// Prevent the case where action group and role share a same name.
return loadConfiguration(CType.ROLES, false).map(
return loadConfiguration(CType.ROLES, false, false).map(
rolesConfiguration -> actionGroupNameIsNotSameAsRoleName(securityConfiguration, rolesConfiguration)
);
}
Expand All @@ -120,12 +128,11 @@ private ValidationResult<SecurityConfiguration> actionGroupNameIsNotSameAsRoleNa
final SecurityConfiguration securityConfiguration,
final SecurityDynamicConfiguration<?> rolesConfiguration
) {
if (rolesConfiguration.getCEntries().containsKey(securityConfiguration.resourceName())) {
if (rolesConfiguration.getCEntries().containsKey(securityConfiguration.entityName())) {
return ValidationResult.error(
RestStatus.BAD_REQUEST,
badRequestMessage(
securityConfiguration.resourceName()
+ " is an existing role. A action group cannot be named with an existing role name."
securityConfiguration.entityName() + " is an existing role. A action group cannot be named with an existing role name."
)
);
}
Expand All @@ -134,23 +141,16 @@ private ValidationResult<SecurityConfiguration> actionGroupNameIsNotSameAsRoleNa

private ValidationResult<SecurityConfiguration> hasSelfReference(final SecurityConfiguration securityConfiguration) throws IOException {
// Prevent the case where action group references to itself in the allowed_actions.
return loadConfiguration(getConfigName(), false).map(actionGroupsConfig -> {
final var actionGroupName = securityConfiguration.resourceName();
final var actionGroup = securityConfiguration.contentAsConfigObject();
actionGroupsConfig.putCObject(securityConfiguration.resourceName(), actionGroup);
if (hasSelfReference(securityConfiguration.resourceName(), actionGroupsConfig)) {
return ValidationResult.error(
RestStatus.BAD_REQUEST,
badRequestMessage(actionGroupName + " cannot be an allowed_action of itself")
);
}
return ValidationResult.success(securityConfiguration);
});
}

private boolean hasSelfReference(final String name, final SecurityDynamicConfiguration<?> configuration) {
List<String> allowedActions = ((ActionGroupsV7) configuration.getCEntry(name)).getAllowed_actions();
return allowedActions.contains(name);
final var actionGroupName = securityConfiguration.entityName();
final var configuration = securityConfiguration.configuration();
final var allowedActions = ((ActionGroupsV7) configuration.getCEntry(actionGroupName)).getAllowed_actions();
if (allowedActions.contains(actionGroupName)) {
return ValidationResult.error(
RestStatus.BAD_REQUEST,
badRequestMessage(actionGroupName + " cannot be an allowed_action of itself")
);
}
return ValidationResult.success(securityConfiguration);
}

@Override
Expand Down Expand Up @@ -186,12 +186,12 @@ public Set<String> mandatoryKeys() {
}

@Override
protected CType getConfigName() {
protected CType getConfigurationType() {
return CType.ACTIONGROUPS;
}

@Override
protected String getResourceName() {
protected String getConfigurationName() {
return "actiongroup";
}

Expand Down
13 changes: 8 additions & 5 deletions src/main/java/org/opensearch/security/dlic/rest/api/AllowlistApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public class AllowlistApiAction extends PatchableResourceApiAction {
new Route(RestRequest.Method.PATCH, "/_plugins/_security/api/allowlist")
);

private static final String RESOURCE_NAME = "config";
private static final String CONFIGURATION_NAME = "config";

@Inject
public AllowlistApiAction(
Expand Down Expand Up @@ -127,7 +127,10 @@ protected void handleApiRequest(final RestChannel channel, final RestRequest req

private void allowlistApiRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) {
requestHandlersBuilder.verifyAccessForAllMethods()
.onChangeRequest(RestRequest.Method.PUT, request -> loadSecurityConfigurationWithRequestContent(RESOURCE_NAME, request))
.onChangeRequest(
RestRequest.Method.PUT,
request -> loadConfigurationWithRequestContent(CONFIGURATION_NAME, request).map(this::addEntityToConfiguration)
)
.override(RestRequest.Method.DELETE, methodNotImplementedHandler);
}

Expand All @@ -137,12 +140,12 @@ public List<Route> routes() {
}

@Override
protected String getResourceName() {
return RESOURCE_NAME;
protected String getConfigurationName() {
return CONFIGURATION_NAME;
}

@Override
protected CType getConfigName() {
protected CType getConfigurationType() {
return CType.ALLOWLIST;
}

Expand Down
44 changes: 23 additions & 21 deletions src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import org.apache.commons.lang3.tuple.Pair;
import org.opensearch.client.Client;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.settings.Settings;
Expand Down Expand Up @@ -139,7 +140,7 @@ public class AuditApiAction extends PatchableResourceApiAction {
)
);

private static final String RESOURCE_NAME = "config";
private static final String CONFIGURATION_NAME = "config";
@VisibleForTesting
public static final String READONLY_FIELD = "_readonly";
@VisibleForTesting
Expand Down Expand Up @@ -261,8 +262,8 @@ protected void handleApiRequest(final RestChannel channel, final RestRequest req
}

@Override
protected String getResourceName() {
return RESOURCE_NAME;
protected String getConfigurationName() {
return CONFIGURATION_NAME;
}

@Override
Expand All @@ -271,7 +272,7 @@ protected Endpoint getEndpoint() {
}

@Override
protected CType getConfigName() {
protected CType getConfigurationType() {
return CType.AUDIT;
}

Expand All @@ -280,38 +281,43 @@ private void auditApiRequestHandlers(RequestHandler.RequestHandlersBuilder reque
requestHandlersBuilder
.onGetRequest(request ->
processGetRequest(request)
.map(securityConfiguration -> {
final var configuration = securityConfiguration.configuration();
.map(configuration -> {
configuration.putCObject(READONLY_FIELD, readonlyFields);
return ValidationResult.success(securityConfiguration);
}))
return ValidationResult.success(configuration);
})
)
.onChangeRequest(RestRequest.Method.PUT, request ->
withConfigResourceNameOnly(request)
.map(ignore -> processPutRequest(request))
.map(this::verifyNotReadonlyFieldUpdated))
.map(config -> loadConfigurationWithRequestContent(config, request))
.map(this::verifyNotReadonlyFieldUpdated)
.map(this::addEntityToConfiguration)
.map(configurationValidators::hasRightsToChangeImmutableEntity)
)
.override(RestRequest.Method.POST, methodNotImplementedHandler)
.override(RestRequest.Method.DELETE, methodNotImplementedHandler);
// spotless:on
}

private ValidationResult<String> withConfigResourceNameOnly(final RestRequest request) {
final var name = nameParam(request);
if (!RESOURCE_NAME.equals(name)) {
return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("name must be config"));
if (!CONFIGURATION_NAME.equals(name)) {
return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("name must be `" + CONFIGURATION_NAME + "`"));
}
return ValidationResult.success(name);
}

private ValidationResult<SecurityConfiguration> verifyNotReadonlyFieldUpdated(final SecurityConfiguration securityConfiguration)
throws IOException {
private ValidationResult<Pair<JsonNode, SecurityConfiguration>> verifyNotReadonlyFieldUpdated(
final Pair<JsonNode, SecurityConfiguration> contentAndSecurityConfiguration
) {
if (!isSuperAdmin()) {
final var existingResource = securityConfiguration.configurationAsJsonNode().get(getResourceName());
final var targetResource = securityConfiguration.requestContent();
final var configuration = contentAndSecurityConfiguration.getRight().configuration();
final var existingResource = Utils.convertJsonToJackson(configuration, false).get(getConfigurationName());
final var targetResource = contentAndSecurityConfiguration.getLeft();
if (readonlyFields.stream().anyMatch(path -> !existingResource.at(path).equals(targetResource.at(path)))) {
return ValidationResult.error(RestStatus.CONFLICT, conflictMessage("Attempted to update read-only property."));
}
}
return ValidationResult.success(securityConfiguration);
return ValidationResult.success(contentAndSecurityConfiguration);
}

@Override
Expand Down Expand Up @@ -342,8 +348,4 @@ protected boolean isReadonlyFieldUpdated(final JsonNode existingResource, final
return false;
}

@Override
protected boolean isReadonlyFieldUpdated(final SecurityDynamicConfiguration<?> configuration, final JsonNode targetResource) {
return isReadonlyFieldUpdated(Utils.convertJsonToJackson(configuration, false).get(getResourceName()), targetResource);
}
}
4 changes: 2 additions & 2 deletions src/main/java/org/opensearch/security/dlic/rest/api/AuthTokenProcessorAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,12 @@ protected RequestContentValidator createValidator(Object... params) {
}

@Override
protected String getResourceName() {
protected String getConfigurationName() {
return "authtoken";
}

@Override
protected CType getConfigName() {
protected CType getConfigurationType() {
return null;
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,13 +130,13 @@ protected RequestContentValidator createValidator(final Object... params) {
}

@Override
protected String getResourceName() {
protected String getConfigurationName() {
// not needed
return null;
}

@Override
protected CType getConfigName() {
protected CType getConfigurationType() {
return null;
}

Expand Down
Loading

0 comments on commit 0e2c1f1

Please sign in to comment.