From 3ccf38be5330aaf046b406c77ea3ec6a61dbaec3 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 23 Feb 2024 22:56:11 +0000 Subject: [PATCH 01/22] Start making a reserved role handler Signed-off-by: Peter Nied --- .../dlic/rest/api/ReservedRolesApiAction.java | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java new file mode 100644 index 0000000000..1e2253fa60 --- /dev/null +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java @@ -0,0 +1,139 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.dlic.rest.api; + +import java.io.IOException; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.stream.StreamSupport; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.fasterxml.jackson.core.JsonPointer; +import com.fasterxml.jackson.databind.JsonNode; +import org.apache.commons.lang3.tuple.Pair; + +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.inject.Inject; +import org.opensearch.common.settings.Settings; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.rest.RestRequest; +import org.opensearch.rest.RestRequest.Method; +import org.opensearch.security.configuration.MaskedField; +import org.opensearch.security.configuration.Salt; +import org.opensearch.security.dlic.rest.api.RolesApiAction.RoleRequestContentValidator; +import org.opensearch.security.dlic.rest.validation.EndpointValidator; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType; +import org.opensearch.security.dlic.rest.validation.ValidationResult; +import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.threadpool.ThreadPool; + +import static org.opensearch.security.dlic.rest.api.RequestHandler.methodNotImplementedHandler; +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; + +public class ReservedRolesApiAction extends AbstractApiAction { + + private static final List routes = addRoutesPrefix( + ImmutableList.of( + new Route(Method.POST, "/roles/_reserved/_upgrade") + ) + ); + + @Inject + public ReservedRolesApiAction( + final ClusterService clusterService, + final ThreadPool threadPool, + final SecurityApiDependencies securityApiDependencies + ) { + super(Endpoint.ROLES, clusterService, threadPool, securityApiDependencies); + this.requestHandlersBuilder.configureRequestHandlers(this::rolesApiRequestHandlers); + } + + @Override + public List routes() { + return routes; + } + + @Override + protected CType getConfigType() { + return CType.ROLES; + } + + private void rolesApiRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) { + requestHandlersBuilder.add(null, methodNotImplementedHandler).onChangeRequest(Method.POST, this::processUpgrade); + } + + protected final ValidationResult processUpgrade(final RestRequest request) throws IOException { + return loadConfiguration(nameParam(request), false).map( + securityConfiguration -> { + final int existingRolesConfig = securityConfiguration.configuration().getCEntry(getConfigType()); + return ValidationResult.success("Upgrade Complete"); + } + ); + } + + @Override + protected EndpointValidator createEndpointValidator() { + return new EndpointValidator() { + + @Override + public Endpoint endpoint() { + return endpoint; + } + + @Override + public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() { + return securityApiDependencies.restApiAdminPrivilegesEvaluator(); + } + + @Override + public ValidationResult isAllowedToChangeImmutableEntity(SecurityConfiguration securityConfiguration) + throws IOException { + return EndpointValidator.super.isAllowedToChangeImmutableEntity(securityConfiguration).map(ignore -> { + if (isCurrentUserAdmin()) { + return ValidationResult.success(securityConfiguration); + } + return isAllowedToChangeEntityWithRestAdminPermissions(securityConfiguration); + }); + } + + @Override + public RequestContentValidator createRequestContentValidator(Object... params) { + return new RoleRequestContentValidator(new RequestContentValidator.ValidationContext() { + @Override + public Object[] params() { + return params; + } + + @Override + public Settings settings() { + return securityApiDependencies.settings(); + } + + @Override + public Map allowedKeys() { + final ImmutableMap.Builder allowedKeys = ImmutableMap.builder(); + if (isCurrentUserAdmin()) allowedKeys.put("reserved", DataType.BOOLEAN); + return allowedKeys.put("cluster_permissions", DataType.ARRAY) + .put("tenant_permissions", DataType.ARRAY) + .put("index_permissions", DataType.ARRAY) + .put("description", DataType.STRING) + .build(); + } + }); + } + }; + } + +} From 7fb4beaf62fbec6e874461d34aa58681dda71c72 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Mon, 26 Feb 2024 20:12:36 +0000 Subject: [PATCH 02/22] Starting to read the file from disk Signed-off-by: Peter Nied --- .../dlic/rest/api/ReservedRolesApiAction.java | 53 +++++++++++++------ .../dlic/rest/api/SecurityConfiguration.java | 2 +- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java index 1e2253fa60..02e43f1a87 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java @@ -11,42 +11,39 @@ package org.opensearch.security.dlic.rest.api; +import static org.opensearch.security.dlic.rest.api.AbstractApiAction.LOGGER; +import static org.opensearch.security.dlic.rest.api.RequestHandler.methodNotImplementedHandler; +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; + import java.io.IOException; +import java.security.AccessController; +import java.security.PrivilegedExceptionAction; import java.util.List; import java.util.Map; -import java.util.Objects; -import java.util.stream.StreamSupport; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.fasterxml.jackson.core.JsonPointer; -import com.fasterxml.jackson.databind.JsonNode; -import org.apache.commons.lang3.tuple.Pair; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.inject.Inject; import org.opensearch.common.settings.Settings; -import org.opensearch.core.rest.RestStatus; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; -import org.opensearch.security.configuration.MaskedField; -import org.opensearch.security.configuration.Salt; import org.opensearch.security.dlic.rest.api.RolesApiAction.RoleRequestContentValidator; import org.opensearch.security.dlic.rest.validation.EndpointValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType; import org.opensearch.security.dlic.rest.validation.ValidationResult; import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; -import static org.opensearch.security.dlic.rest.api.RequestHandler.methodNotImplementedHandler; -import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; public class ReservedRolesApiAction extends AbstractApiAction { private static final List routes = addRoutesPrefix( ImmutableList.of( - new Route(Method.POST, "/roles/_reserved/_upgrade") + new Route(Method.GET, "/roles/_reserved/_upgrade_check"), + new Route(Method.POST, "/roles/_reserved/_upgrade_apply") ) ); @@ -57,9 +54,35 @@ public ReservedRolesApiAction( final SecurityApiDependencies securityApiDependencies ) { super(Endpoint.ROLES, clusterService, threadPool, securityApiDependencies); - this.requestHandlersBuilder.configureRequestHandlers(this::rolesApiRequestHandlers); + this.requestHandlersBuilder.configureRequestHandlers(rhb -> { + rhb.onGetRequest(request -> + withIOException(() -> + loadConfiguration(getConfigType(), false, false) + .map(securityConfiguration -> { + try { + final var entries = securityConfiguration.getCEntries(); + final var entriesJson = convertJsonToJackson(entries); + + Utils. + } catch (final Exception ex) { + // log it ? + } + return (JsonNode)null; + })))); } + public static void uploadFile( + String filepath, + String index, + CType cType, + ) { + final String configType = cType.toLCString(); + + var fromDisk = AccessController.doPrivileged((PrivilegedExceptionAction) () -> + ConfigHelper.fromYamlFile(filepath, cType, configVersion, 0, 0)); + } + + @Override public List routes() { return routes; diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfiguration.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfiguration.java index 4d33e42fad..438c59877c 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfiguration.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfiguration.java @@ -76,7 +76,7 @@ public static SecurityConfiguration of(final String entityName, final SecurityDy public static SecurityConfiguration of( final JsonNode requestContent, final String entityName, - final SecurityDynamicConfiguration configuration + final SecurityDynamicConfiguration configuration ) { Objects.requireNonNull(configuration, "configuration hasn't been set"); Objects.requireNonNull(requestContent, "requestContent hasn't been set"); From 745eeb7f3096d737787c0a03c1090005b136ab8b Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 1 Mar 2024 23:12:44 +0000 Subject: [PATCH 03/22] Functional diff Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 75 ++++++-- src/integrationTest/resources/roles.yml | 18 ++ .../ConfigurationRepository.java | 13 +- .../dlic/rest/api/ConfigUpgradeApiAction.java | 172 ++++++++++++++++++ .../dlic/rest/api/ReservedRolesApiAction.java | 162 ----------------- .../dlic/rest/api/SecurityRestApiActions.java | 3 +- .../rest/validation/ValidationResult.java | 3 + 7 files changed, 265 insertions(+), 181 deletions(-) create mode 100644 src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java delete mode 100644 src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index 8bb5b96145..b254e3182a 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -11,17 +11,22 @@ import java.io.IOException; import java.nio.file.Path; +import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Stream; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import com.fasterxml.jackson.databind.JsonNode; + import org.apache.commons.io.FileUtils; import org.awaitility.Awaitility; import org.junit.AfterClass; import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; - +import org.opensearch.test.framework.TestSecurityConfig.User; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; @@ -38,10 +43,9 @@ public class DefaultConfigurationTests { private final static Path configurationFolder = ConfigurationFiles.createConfigurationDirectory(); - public static final String ADMIN_USER_NAME = "admin"; - public static final String DEFAULT_PASSWORD = "secret"; - public static final String NEW_USER = "new-user"; - public static final String LIMITED_USER = "limited-user"; + private static final User ADMIN_USER = new User("admin"); + private static final User NEW_USER = new User("new-user"); + private static final User LIMITED_USER = new User("limited-user"); @ClassRule public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) @@ -62,17 +66,60 @@ public static void cleanConfigurationDirectory() throws IOException { FileUtils.deleteDirectory(configurationFolder.toFile()); } + // @Test + // public void shouldLoadDefaultConfiguration() { + // try (TestRestClient client = cluster.getRestClient(NEW_USER)) { + // Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); + // } + // try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + // client.confirmCorrectCredentials(ADMIN_USER.getName()); + // HttpResponse response = client.get("_plugins/_security/api/internalusers"); + // response.assertStatusCode(200); + // Map users = response.getBodyAs(Map.class); + // assertThat(users, allOf(aMapWithSize(3), hasKey(ADMIN_USER.getName()), hasKey(NEW_USER.getName()), hasKey(LIMITED_USER.getName()))); + // } + // } + + @Test - public void shouldLoadDefaultConfiguration() { - try (TestRestClient client = cluster.getRestClient(NEW_USER, DEFAULT_PASSWORD)) { + public void securityRolesUgrade() throws Exception { + try (var client = cluster.getRestClient(ADMIN_USER)) { Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); + + final var defaultRolesResponse = client.get("_plugins/_security/api/roles/"); + final var roles = defaultRolesResponse.getBodyAs(JsonNode.class); + final var rolesCount = extractFieldNames(roles).size(); + + final var checkForUpgrade = client.get("_plugins/_security/api/_upgrade_check"); + System.out.println("checkForUpgrade Response: " + checkForUpgrade.getBody()); + + + final var roleToDelete = "flow_framework_full_access"; + final var deleteRoleResponse = client.delete("_plugins/_security/api/roles/" + roleToDelete); + deleteRoleResponse.assertStatusCode(200); + + final var checkForUpgrade3 = client.get("_plugins/_security/api/_upgrade_check"); + System.out.println("checkForUpgrade3 Response: " + checkForUpgrade3.getBody()); + + final var roleToAlter = "flow_framework_read_access"; + final String patchBody = "[{ \"op\": \"replace\", \"path\": \"/cluster_permissions\", \"value\":" + + "[\"a\",\"b\",\"c\"]" + + "},{ \"op\": \"add\", \"path\": \"/index_permissions\", \"value\":" + + "[{\"index_patterns\":[\"*\"],\"allowed_actions\":[\"*\"]}]" + + "}]"; + final var updateRoleResponse = client.patch("_plugins/_security/api/roles/" + roleToAlter, patchBody); + updateRoleResponse.assertStatusCode(200); + System.out.println("Updated Role Response: " +updateRoleResponse.getBody()); + + final var checkForUpgrade2 = client.get("_plugins/_security/api/_upgrade_check"); + System.out.println("checkForUpgrade2 Response: " + checkForUpgrade2.getBody()); + } - try (TestRestClient client = cluster.getRestClient(ADMIN_USER_NAME, DEFAULT_PASSWORD)) { - client.confirmCorrectCredentials(ADMIN_USER_NAME); - HttpResponse response = client.get("_plugins/_security/api/internalusers"); - response.assertStatusCode(200); - Map users = response.getBodyAs(Map.class); - assertThat(users, allOf(aMapWithSize(3), hasKey(ADMIN_USER_NAME), hasKey(NEW_USER), hasKey(LIMITED_USER))); - } + } + + private List extractFieldNames(final JsonNode json) { + final var list = new ArrayList(); + json.fieldNames().forEachRemaining(list::add); + return list; } } diff --git a/src/integrationTest/resources/roles.yml b/src/integrationTest/resources/roles.yml index 02de9bf3d5..2ea7548ad6 100644 --- a/src/integrationTest/resources/roles.yml +++ b/src/integrationTest/resources/roles.yml @@ -17,3 +17,21 @@ user_limited-user__limited-role: allowed_actions: - "indices:data/read/get" - "indices:data/read/search" +flow_framework_full_access: + cluster_permissions: + - 'cluster:admin/opensearch/flow_framework/*' + - 'cluster_monitor' + index_permissions: + - index_patterns: + - '*' + allowed_actions: + - 'indices:admin/aliases/get' + - 'indices:admin/mappings/get' + - 'indices_monitor' +flow_framework_read_access: + cluster_permissions: + - 'cluster:admin/opensearch/flow_framework/workflow/get' + - 'cluster:admin/opensearch/flow_framework/workflow/search' + - 'cluster:admin/opensearch/flow_framework/workflow_state/get' + - 'cluster:admin/opensearch/flow_framework/workflow_state/search' + - 'cluster:admin/opensearch/flow_framework/workflow_step/get' diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index dfbeb16cb3..353286fc4a 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -123,6 +123,14 @@ private ConfigurationRepository( configCache = CacheBuilder.newBuilder().build(); } + public String getConfigDirectory() { + String lookupDir = System.getProperty("security.default_init.dir"); + final String cd = lookupDir != null + ? (lookupDir + "/") + : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; + return cd; + } + private void initalizeClusterConfiguration(final boolean installDefaultConfig) { try { LOGGER.info("Background init thread started. Install default config?: " + installDefaultConfig); @@ -135,10 +143,7 @@ private void initalizeClusterConfiguration(final boolean installDefaultConfig) { if (installDefaultConfig) { try { - String lookupDir = System.getProperty("security.default_init.dir"); - final String cd = lookupDir != null - ? (lookupDir + "/") - : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; + final String cd = getConfigDirectory(); File confFile = new File(cd + "config.yml"); if (confFile.exists()) { final ThreadContext threadContext = threadPool.getThreadContext(); diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java new file mode 100644 index 0000000000..4090a6b38c --- /dev/null +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -0,0 +1,172 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.dlic.rest.api; + + + +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; +import static org.opensearch.security.dlic.rest.support.Utils.withIOException; + +import java.io.IOException; +import java.security.AccessController; +import java.security.PrivilegedActionException; +import java.security.PrivilegedExceptionAction; +import java.util.EnumSet; +import java.util.List; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.opensearch.client.Client; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.inject.Inject; +import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestRequest; +import org.opensearch.rest.RestRequest.Method; +import org.opensearch.security.configuration.ConfigurationRepository; +import org.opensearch.security.dlic.rest.support.Utils; +import org.opensearch.security.dlic.rest.validation.ValidationResult; +import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.support.ConfigHelper; +import org.opensearch.threadpool.ThreadPool; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; +import com.flipkart.zjsonpatch.DiffFlags; +import com.flipkart.zjsonpatch.JsonDiff; +import com.google.common.collect.ImmutableList; + +public class ConfigUpgradeApiAction extends AbstractApiAction { + + private final static Logger LOGGER = LogManager.getLogger(ConfigUpgradeApiAction.class); + + private static final List routes = addRoutesPrefix( + ImmutableList.of( + new Route(Method.GET, "/_upgrade_check") + ) + ); + + @Inject + public ConfigUpgradeApiAction( + final ClusterService clusterService, + final ThreadPool threadPool, + final SecurityApiDependencies securityApiDependencies + ) { + super(Endpoint.CONFIG, clusterService, threadPool, securityApiDependencies); + this.requestHandlersBuilder.configureRequestHandlers(rhb -> { + rhb.add(Method.GET, this::handleRolesCanUpgrade); + }); + } + + public void handleRolesCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) { + try { + withIOException(() -> computeDifferenceToUpdate(CType.ROLES, "roles.yml") + .map(differences -> { + final var canUpgrade = differences.size() > 0; + + // Step 4: Return a response indicating if an upgrade can be performed + ObjectNode response = JsonNodeFactory.instance.objectNode(); + response.put("can_upgrade", canUpgrade); + + if (canUpgrade) { + // Optionally include the differences in the response + response.set("differences", differences); + } + return ValidationResult.success(response); + })); +// Handle how this is returned! + channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); + } catch (Exception e) { + // Handle other exceptions + LOGGER.error("Unexpected error during upgrade check", e); + channel.sendResponse(new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, "{\"error\":\"Unexpected error checking for upgrade\"}")); + } + } + + private ValidationResult computeDifferenceToUpdate(final CType configType, final String configName) throws IOException { + return + loadConfiguration(configType, false, false) + .map(activeRoles -> { + final var activeRolesJson = Utils.convertJsonToJackson(activeRoles, false); + final var defaultRolesJson = loadConfigFileAsJson(configName, configType); + final var rawDiff = JsonDiff.asJson(activeRolesJson, defaultRolesJson, EnumSet.of(DiffFlags.OMIT_VALUE_ON_REMOVE)); + return ValidationResult.success(filterRemoveOperations(rawDiff)); + }); + } + + private JsonNode filterRemoveOperations(final JsonNode diff) { + final ArrayNode filteredDiff = JsonNodeFactory.instance.arrayNode(); + diff.forEach(node -> { + if (!isRemoveOperation(node)) { + filteredDiff.add(node); + return; + } else { + if (!hasRootLevelPath(node)) { + filteredDiff.add(node); + } + } + }); + return filteredDiff; + } + + private boolean hasRootLevelPath(final JsonNode node) { + final var jsonPath = node.get("path").asText(); + return jsonPath.charAt(0 ) == '/' && !jsonPath.substring(1).contains("/"); + } + private boolean isRemoveOperation(final JsonNode node) { + return node.get("op").asText().equals("remove"); + } + + public JsonNode loadConfigFileAsJson(final String fileName, final CType cType) { + final var cd = securityApiDependencies.configurationRepository().getConfigDirectory(); + final var filepath = cd + fileName; + try { + return AccessController.doPrivileged((PrivilegedExceptionAction) () -> { + var loadedConfiguration = ConfigHelper.fromYamlFile(filepath, cType, ConfigurationRepository.DEFAULT_CONFIG_VERSION, 0, 0); + return Utils.convertJsonToJackson(loadedConfiguration, false); + }); + } catch (PrivilegedActionException e) { + // TODO Auto-generated catch block + e.printStackTrace(); + throw new RuntimeException(e); + } + } + + + @Override + public List routes() { + return routes; + } + + @Override + protected CType getConfigType() { + return CType.ROLES; + } + + // private void rolesApiRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) { + // requestHandlersBuilder.add(null, methodNotImplementedHandler).onChangeRequest(Method.POST, this::processUpgrade); + // } + + // protected final ValidationResult processUpgrade(final RestRequest request) throws IOException { + // return loadConfiguration(nameParam(request), false).map( + // securityConfiguration -> { + // final int existingRolesConfig = securityConfiguration.configuration().getCEntry(getConfigType()); + // return ValidationResult.success("Upgrade Complete"); + // } + // ); + // } + +} diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java deleted file mode 100644 index 02e43f1a87..0000000000 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ReservedRolesApiAction.java +++ /dev/null @@ -1,162 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - * - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - -package org.opensearch.security.dlic.rest.api; - -import static org.opensearch.security.dlic.rest.api.AbstractApiAction.LOGGER; -import static org.opensearch.security.dlic.rest.api.RequestHandler.methodNotImplementedHandler; -import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; - -import java.io.IOException; -import java.security.AccessController; -import java.security.PrivilegedExceptionAction; -import java.util.List; -import java.util.Map; - -import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.inject.Inject; -import org.opensearch.common.settings.Settings; -import org.opensearch.rest.RestRequest; -import org.opensearch.rest.RestRequest.Method; -import org.opensearch.security.dlic.rest.api.RolesApiAction.RoleRequestContentValidator; -import org.opensearch.security.dlic.rest.validation.EndpointValidator; -import org.opensearch.security.dlic.rest.validation.RequestContentValidator; -import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType; -import org.opensearch.security.dlic.rest.validation.ValidationResult; -import org.opensearch.security.securityconf.impl.CType; -import org.opensearch.security.support.ConfigHelper; -import org.opensearch.threadpool.ThreadPool; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; - -public class ReservedRolesApiAction extends AbstractApiAction { - - private static final List routes = addRoutesPrefix( - ImmutableList.of( - new Route(Method.GET, "/roles/_reserved/_upgrade_check"), - new Route(Method.POST, "/roles/_reserved/_upgrade_apply") - ) - ); - - @Inject - public ReservedRolesApiAction( - final ClusterService clusterService, - final ThreadPool threadPool, - final SecurityApiDependencies securityApiDependencies - ) { - super(Endpoint.ROLES, clusterService, threadPool, securityApiDependencies); - this.requestHandlersBuilder.configureRequestHandlers(rhb -> { - rhb.onGetRequest(request -> - withIOException(() -> - loadConfiguration(getConfigType(), false, false) - .map(securityConfiguration -> { - try { - final var entries = securityConfiguration.getCEntries(); - final var entriesJson = convertJsonToJackson(entries); - - Utils. - } catch (final Exception ex) { - // log it ? - } - return (JsonNode)null; - })))); - } - - public static void uploadFile( - String filepath, - String index, - CType cType, - ) { - final String configType = cType.toLCString(); - - var fromDisk = AccessController.doPrivileged((PrivilegedExceptionAction) () -> - ConfigHelper.fromYamlFile(filepath, cType, configVersion, 0, 0)); - } - - - @Override - public List routes() { - return routes; - } - - @Override - protected CType getConfigType() { - return CType.ROLES; - } - - private void rolesApiRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) { - requestHandlersBuilder.add(null, methodNotImplementedHandler).onChangeRequest(Method.POST, this::processUpgrade); - } - - protected final ValidationResult processUpgrade(final RestRequest request) throws IOException { - return loadConfiguration(nameParam(request), false).map( - securityConfiguration -> { - final int existingRolesConfig = securityConfiguration.configuration().getCEntry(getConfigType()); - return ValidationResult.success("Upgrade Complete"); - } - ); - } - - @Override - protected EndpointValidator createEndpointValidator() { - return new EndpointValidator() { - - @Override - public Endpoint endpoint() { - return endpoint; - } - - @Override - public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() { - return securityApiDependencies.restApiAdminPrivilegesEvaluator(); - } - - @Override - public ValidationResult isAllowedToChangeImmutableEntity(SecurityConfiguration securityConfiguration) - throws IOException { - return EndpointValidator.super.isAllowedToChangeImmutableEntity(securityConfiguration).map(ignore -> { - if (isCurrentUserAdmin()) { - return ValidationResult.success(securityConfiguration); - } - return isAllowedToChangeEntityWithRestAdminPermissions(securityConfiguration); - }); - } - - @Override - public RequestContentValidator createRequestContentValidator(Object... params) { - return new RoleRequestContentValidator(new RequestContentValidator.ValidationContext() { - @Override - public Object[] params() { - return params; - } - - @Override - public Settings settings() { - return securityApiDependencies.settings(); - } - - @Override - public Map allowedKeys() { - final ImmutableMap.Builder allowedKeys = ImmutableMap.builder(); - if (isCurrentUserAdmin()) allowedKeys.put("reserved", DataType.BOOLEAN); - return allowedKeys.put("cluster_permissions", DataType.ARRAY) - .put("tenant_permissions", DataType.ARRAY) - .put("index_permissions", DataType.ARRAY) - .put("description", DataType.STRING) - .build(); - } - }); - } - }; - } - -} diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java index b0d46f8774..f38cf0580d 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java @@ -95,7 +95,8 @@ public static Collection getHandler( new AllowlistApiAction(Endpoint.ALLOWLIST, clusterService, threadPool, securityApiDependencies), new AuditApiAction(clusterService, threadPool, securityApiDependencies), new MultiTenancyConfigApiAction(clusterService, threadPool, securityApiDependencies), - new SecuritySSLCertsApiAction(clusterService, threadPool, securityKeyStore, certificatesReloadEnabled, securityApiDependencies) + new SecuritySSLCertsApiAction(clusterService, threadPool, securityKeyStore, certificatesReloadEnabled, securityApiDependencies), + new ConfigUpgradeApiAction(clusterService, threadPool, securityApiDependencies) ); } diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java index ea782ea504..8398fbd7a4 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java @@ -83,4 +83,7 @@ public ToXContent errorMessage() { return errorMessage; } + public C getContent(){ + return content; + } } From c8db0ec11fa825eb88e16c9c78a949f890a02d1a Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 5 Mar 2024 01:13:37 +0000 Subject: [PATCH 04/22] Functional upgrade check Signed-off-by: Peter Nied --- .../dlic/rest/api/ConfigUpgradeApiAction.java | 196 +++++++++++++----- .../dlic/rest/api/SecurityConfiguration.java | 2 +- .../rest/validation/ValidationResult.java | 17 +- 3 files changed, 155 insertions(+), 60 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 4090a6b38c..56e9ec49d0 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -11,22 +11,52 @@ package org.opensearch.security.dlic.rest.api; - - +import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; +import static org.opensearch.security.dlic.rest.api.Responses.ok; +import static org.opensearch.security.dlic.rest.api.Responses.response; import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; import static org.opensearch.security.dlic.rest.support.Utils.withIOException; import java.io.IOException; +import java.nio.file.Path; import java.security.AccessController; import java.security.PrivilegedActionException; import java.security.PrivilegedExceptionAction; +import java.util.ArrayList; import java.util.EnumSet; import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.collect.Tuple; +import org.opensearch.common.inject.Inject; +import org.opensearch.common.settings.Settings; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestRequest; +import org.opensearch.rest.RestRequest.Method; +import org.opensearch.security.configuration.ConfigurationRepository; +import org.opensearch.security.dlic.rest.support.Utils; +import org.opensearch.security.dlic.rest.validation.EndpointValidator; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType; +import org.opensearch.security.dlic.rest.validation.ValidationResult; +import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.support.ConfigHelper; +import org.opensearch.threadpool.ThreadPool; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.flipkart.zjsonpatch.DiffFlags; +import com.flipkart.zjsonpatch.JsonDiff; +import com.google.common.collect.ImmutableList; import org.opensearch.common.inject.Inject; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.rest.RestStatus; @@ -49,15 +79,14 @@ import com.flipkart.zjsonpatch.JsonDiff; import com.google.common.collect.ImmutableList; + public class ConfigUpgradeApiAction extends AbstractApiAction { private final static Logger LOGGER = LogManager.getLogger(ConfigUpgradeApiAction.class); - private static final List routes = addRoutesPrefix( - ImmutableList.of( - new Route(Method.GET, "/_upgrade_check") - ) - ); + private static final List routes = addRoutesPrefix(ImmutableList.of( + new Route(Method.GET, "/_upgrade_check"), + new Route(Method.POST, "/_upgrade_perform"))); @Inject public ConfigUpgradeApiAction( @@ -67,55 +96,80 @@ public ConfigUpgradeApiAction( ) { super(Endpoint.CONFIG, clusterService, threadPool, securityApiDependencies); this.requestHandlersBuilder.configureRequestHandlers(rhb -> { - rhb.add(Method.GET, this::handleRolesCanUpgrade); + rhb.allMethodsNotImplemented().add(Method.GET, this::handleCanUpgrade).add(Method.POST, this::handleUpgrade); }); } - public void handleRolesCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) { - try { - withIOException(() -> computeDifferenceToUpdate(CType.ROLES, "roles.yml") - .map(differences -> { - final var canUpgrade = differences.size() > 0; - - // Step 4: Return a response indicating if an upgrade can be performed - ObjectNode response = JsonNodeFactory.instance.objectNode(); - response.put("can_upgrade", canUpgrade); - - if (canUpgrade) { - // Optionally include the differences in the response - response.set("differences", differences); - } - return ValidationResult.success(response); - })); -// Handle how this is returned! - channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); - } catch (Exception e) { - // Handle other exceptions - LOGGER.error("Unexpected error during upgrade check", e); - channel.sendResponse(new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, "{\"error\":\"Unexpected error checking for upgrade\"}")); + void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { + withIOException(() -> getConfigurations(request) + .map(configurations -> { + final var differencesList = new ArrayList>>(); + for (final var configuration : configurations) { + differencesList.add(computeDifferenceToUpdate(configuration) + .map(differences -> ValidationResult.success(new Tuple(configuration, differences.deepCopy())))); + } + return ValidationResult.combine(differencesList); + })) + .valid(differencesList -> { + final var canUpgrade = differencesList.stream().anyMatch(entry -> entry.v2().size() > 0); + + final ObjectNode response = JsonNodeFactory.instance.objectNode(); + response.put("can_upgrade", canUpgrade); + + if (canUpgrade) { + final ObjectNode differences = JsonNodeFactory.instance.objectNode(); + differencesList.forEach(t -> { + differences.put(t.v1().toLCString(), t.v2()); + }); + response.put("differences", differences); + } + channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); + }) + .error((status, toXContent) -> response(channel, status, toXContent)); + } + + private void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { + throw new UnsupportedOperationException("Unimplemented method 'handleUpgrade'"); + } + + private ValidationResult computeDifferenceToUpdate(final CType configType) throws IOException { + return loadConfiguration(configType, false, false).map(activeRoles -> { + final var activeRolesJson = Utils.convertJsonToJackson(activeRoles, false); + final var defaultRolesJson = loadConfigFileAsJson(configType); + final var rawDiff = JsonDiff.asJson(activeRolesJson, defaultRolesJson, EnumSet.of(DiffFlags.OMIT_VALUE_ON_REMOVE)); + return ValidationResult.success(filterRemoveOperations(rawDiff)); + }); + } + + private ValidationResult> getConfigurations(final RestRequest request) { + final String[] configs = request.paramAsStringArray("configs", null); + + final var configurations = Optional.ofNullable(configs) + .map(CType::fromStringValues) + .orElse(supportedConfigs()); + + if (!configurations.stream().allMatch(supportedConfigs()::contains)) { + // Remove all supported configurations + configurations.removeAll(supportedConfigs()); + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Unsupported configurations for upgrade" + configurations)); } + + return ValidationResult.success(configurations); } - private ValidationResult computeDifferenceToUpdate(final CType configType, final String configName) throws IOException { - return - loadConfiguration(configType, false, false) - .map(activeRoles -> { - final var activeRolesJson = Utils.convertJsonToJackson(activeRoles, false); - final var defaultRolesJson = loadConfigFileAsJson(configName, configType); - final var rawDiff = JsonDiff.asJson(activeRolesJson, defaultRolesJson, EnumSet.of(DiffFlags.OMIT_VALUE_ON_REMOVE)); - return ValidationResult.success(filterRemoveOperations(rawDiff)); - }); + private Set supportedConfigs() { + return Set.of(CType.ROLES); } private JsonNode filterRemoveOperations(final JsonNode diff) { final ArrayNode filteredDiff = JsonNodeFactory.instance.arrayNode(); diff.forEach(node -> { if (!isRemoveOperation(node)) { - filteredDiff.add(node); + filteredDiff.add(node.deepCopy()); return; } else { if (!hasRootLevelPath(node)) { - filteredDiff.add(node); + filteredDiff.add(node.deepCopy()); } } }); @@ -124,28 +178,28 @@ private JsonNode filterRemoveOperations(final JsonNode diff) { private boolean hasRootLevelPath(final JsonNode node) { final var jsonPath = node.get("path").asText(); - return jsonPath.charAt(0 ) == '/' && !jsonPath.substring(1).contains("/"); + return jsonPath.charAt(0) == '/' && !jsonPath.substring(1).contains("/"); } + private boolean isRemoveOperation(final JsonNode node) { return node.get("op").asText().equals("remove"); } - public JsonNode loadConfigFileAsJson(final String fileName, final CType cType) { + public JsonNode loadConfigFileAsJson(final CType cType) { final var cd = securityApiDependencies.configurationRepository().getConfigDirectory(); - final var filepath = cd + fileName; + final var filepath = cType.configFile(Path.of(cd)).toString(); try { return AccessController.doPrivileged((PrivilegedExceptionAction) () -> { var loadedConfiguration = ConfigHelper.fromYamlFile(filepath, cType, ConfigurationRepository.DEFAULT_CONFIG_VERSION, 0, 0); return Utils.convertJsonToJackson(loadedConfiguration, false); }); - } catch (PrivilegedActionException e) { + } catch (final PrivilegedActionException e) { // TODO Auto-generated catch block e.printStackTrace(); throw new RuntimeException(e); } } - @Override public List routes() { return routes; @@ -156,17 +210,45 @@ protected CType getConfigType() { return CType.ROLES; } - // private void rolesApiRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) { - // requestHandlersBuilder.add(null, methodNotImplementedHandler).onChangeRequest(Method.POST, this::processUpgrade); - // } + @Override + protected EndpointValidator createEndpointValidator() { + return new EndpointValidator() { - // protected final ValidationResult processUpgrade(final RestRequest request) throws IOException { - // return loadConfiguration(nameParam(request), false).map( - // securityConfiguration -> { - // final int existingRolesConfig = securityConfiguration.configuration().getCEntry(getConfigType()); - // return ValidationResult.success("Upgrade Complete"); - // } - // ); - // } + @Override + public Endpoint endpoint() { + return endpoint; + } + @Override + public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() { + return securityApiDependencies.restApiAdminPrivilegesEvaluator(); + } + + @Override + public RequestContentValidator createRequestContentValidator(final Object... params) { + return RequestContentValidator.of(new RequestContentValidator.ValidationContext() { + + @Override + public Set mandatoryKeys() { + return Set.of("configs"); + } + + @Override + public Map allowedKeys() { + return Map.of("configs", DataType.ARRAY); + } + + @Override + public Object[] params() { + return params; + } + + @Override + public Settings settings() { + return securityApiDependencies.settings(); + } + }); + } + }; + } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfiguration.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfiguration.java index 438c59877c..4d33e42fad 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfiguration.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfiguration.java @@ -76,7 +76,7 @@ public static SecurityConfiguration of(final String entityName, final SecurityDy public static SecurityConfiguration of( final JsonNode requestContent, final String entityName, - final SecurityDynamicConfiguration configuration + final SecurityDynamicConfiguration configuration ) { Objects.requireNonNull(configuration, "configuration hasn't been set"); Objects.requireNonNull(requestContent, "requestContent hasn't been set"); diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java index 8398fbd7a4..b0d2bf1a73 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java @@ -12,6 +12,8 @@ package org.opensearch.security.dlic.rest.validation; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import java.util.Objects; import org.opensearch.common.CheckedBiConsumer; @@ -50,6 +52,17 @@ public static ValidationResult error(final RestStatus status, final ToXCo return new ValidationResult<>(status, errorMessage); } + public static ValidationResult> combine(final List> entries) { + final var returnList = new ArrayList(); + for (final var entry : entries) { + if (!entry.isValid()) { + return error(entry.status(), entry.errorMessage()); + } + returnList.add(entry.content); + } + return success(returnList); + } + public ValidationResult map(final CheckedFunction, IOException> mapper) throws IOException { if (content != null) { return Objects.requireNonNull(mapper).apply(content); @@ -83,7 +96,7 @@ public ToXContent errorMessage() { return errorMessage; } - public C getContent(){ - return content; + public C getContent() { + return content; } } From dd099bea17d959d528eaed9d254b404e25e3d568 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 6 Mar 2024 23:53:04 +0000 Subject: [PATCH 05/22] [Cherry-Pick] Configuration Type improvements from 'willyborankin/init-index-on-managed-node' Signed-off-by: Andrey Pleskach Signed-off-by: Peter Nied --- .../security/securityconf/impl/CType.java | 59 +++++++++++++------ 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/opensearch/security/securityconf/impl/CType.java b/src/main/java/org/opensearch/security/securityconf/impl/CType.java index 4e5e2de496..23158e5850 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/CType.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/CType.java @@ -27,14 +27,17 @@ package org.opensearch.security.securityconf.impl; +import java.nio.file.Path; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; +import com.google.common.collect.ImmutableMap; + import org.opensearch.security.auditlog.config.AuditConfig; import org.opensearch.security.securityconf.impl.v6.ActionGroupsV6; import org.opensearch.security.securityconf.impl.v6.ConfigV6; @@ -50,21 +53,39 @@ public enum CType { - INTERNALUSERS(toMap(1, InternalUserV6.class, 2, InternalUserV7.class)), - ACTIONGROUPS(toMap(0, List.class, 1, ActionGroupsV6.class, 2, ActionGroupsV7.class)), - CONFIG(toMap(1, ConfigV6.class, 2, ConfigV7.class)), - ROLES(toMap(1, RoleV6.class, 2, RoleV7.class)), - ROLESMAPPING(toMap(1, RoleMappingsV6.class, 2, RoleMappingsV7.class)), - TENANTS(toMap(2, TenantV7.class)), - NODESDN(toMap(1, NodesDn.class, 2, NodesDn.class)), - WHITELIST(toMap(1, WhitelistingSettings.class, 2, WhitelistingSettings.class)), - ALLOWLIST(toMap(1, AllowlistingSettings.class, 2, AllowlistingSettings.class)), - AUDIT(toMap(1, AuditConfig.class, 2, AuditConfig.class)); + ACTIONGROUPS(toMap(0, List.class, 1, ActionGroupsV6.class, 2, ActionGroupsV7.class), "action_groups.yml", false), + ALLOWLIST(toMap(1, AllowlistingSettings.class, 2, AllowlistingSettings.class), "allowlist.yml", true), + AUDIT(toMap(1, AuditConfig.class, 2, AuditConfig.class), "audit.yml", true), + CONFIG(toMap(1, ConfigV6.class, 2, ConfigV7.class), "config.yml", false), + INTERNALUSERS(toMap(1, InternalUserV6.class, 2, InternalUserV7.class), "internal_users.yml", false), + NODESDN(toMap(1, NodesDn.class, 2, NodesDn.class), "nodes_dn.yml", true), + ROLES(toMap(1, RoleV6.class, 2, RoleV7.class), "roles.yml", false), + ROLESMAPPING(toMap(1, RoleMappingsV6.class, 2, RoleMappingsV7.class), "roles_mapping.yml", false), + TENANTS(toMap(2, TenantV7.class), "tenants.yml", false), + WHITELIST(toMap(1, WhitelistingSettings.class, 2, WhitelistingSettings.class), "whitelist.yml", true); + + public static final List REQUIRED_CONFIG_FILES = Arrays.stream(CType.values()) + .filter(Predicate.not(CType::emptyIfMissing)) + .collect(Collectors.toList()); + + public static final List NOT_REQUIRED_CONFIG_FILES = Arrays.stream(CType.values()) + .filter(CType::emptyIfMissing) + .collect(Collectors.toList()); + + private final Map> implementations; + + private final String configFileName; - private Map> implementations; + private final boolean emptyIfMissing; - private CType(Map> implementations) { + private CType(Map> implementations, final String configFileName, final boolean emptyIfMissing) { this.implementations = implementations; + this.configFileName = configFileName; + this.emptyIfMissing = emptyIfMissing; + } + + public boolean emptyIfMissing() { + return emptyIfMissing; } public Map> getImplementationClass() { @@ -80,18 +101,22 @@ public String toLCString() { } public static Set lcStringValues() { - return Arrays.stream(CType.values()).map(c -> c.toLCString()).collect(Collectors.toSet()); + return Arrays.stream(CType.values()).map(CType::toLCString).collect(Collectors.toSet()); } public static Set fromStringValues(String[] strings) { - return Arrays.stream(strings).map(c -> CType.fromString(c)).collect(Collectors.toSet()); + return Arrays.stream(strings).map(CType::fromString).collect(Collectors.toSet()); + } + + public Path configFile(final Path configDir) { + return configDir.resolve(this.configFileName); } private static Map> toMap(Object... objects) { - final Map> map = new HashMap>(); + final ImmutableMap.Builder> map = ImmutableMap.builder(); for (int i = 0; i < objects.length; i = i + 2) { map.put((Integer) objects[i], (Class) objects[i + 1]); } - return Collections.unmodifiableMap(map); + return map.build(); } } From 20acbba805f5d0f31b21f7abde2acbb4d0cc4000 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 00:05:25 +0000 Subject: [PATCH 06/22] Cleaning up Signed-off-by: Peter Nied --- .../dlic/rest/api/ConfigUpgradeApiAction.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 56e9ec49d0..18e88b3d62 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -51,6 +51,7 @@ import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.JsonNodeFactory; @@ -101,7 +102,7 @@ public ConfigUpgradeApiAction( } void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { - withIOException(() -> getConfigurations(request) + withIOException(() -> getAndValidateConfigurationsToUpgrade(request) .map(configurations -> { final var differencesList = new ArrayList>>(); for (final var configuration : configurations) { @@ -141,7 +142,7 @@ private ValidationResult computeDifferenceToUpdate(final CType configT }); } - private ValidationResult> getConfigurations(final RestRequest request) { + private ValidationResult> getAndValidateConfigurationsToUpgrade(final RestRequest request) { final String[] configs = request.paramAsStringArray("configs", null); final var configurations = Optional.ofNullable(configs) @@ -185,7 +186,7 @@ private boolean isRemoveOperation(final JsonNode node) { return node.get("op").asText().equals("remove"); } - public JsonNode loadConfigFileAsJson(final CType cType) { + public JsonNode loadConfigFileAsJson(final CType cType) throws IOException { final var cd = securityApiDependencies.configurationRepository().getConfigDirectory(); final var filepath = cType.configFile(Path.of(cd)).toString(); try { @@ -194,9 +195,8 @@ public JsonNode loadConfigFileAsJson(final CType cType) { return Utils.convertJsonToJackson(loadedConfiguration, false); }); } catch (final PrivilegedActionException e) { - // TODO Auto-generated catch block - e.printStackTrace(); - throw new RuntimeException(e); + LOGGER.error("Error when loading configuration from file", e); + throw (IOException) e.getCause(); } } From cafb7769a1a0dcde20b25bf367f7a8899010f5bc Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 5 Mar 2024 01:13:37 +0000 Subject: [PATCH 07/22] Functional upgrade check Signed-off-by: Peter Nied From d2843cff0ab46d4548c0a2d059f5461def2f9d3a Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 5 Mar 2024 04:58:50 -0600 Subject: [PATCH 08/22] Remove unneed deep copies Signed-off-by: Peter Nied --- .../security/dlic/rest/api/ConfigUpgradeApiAction.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 18e88b3d62..6b81876af0 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -107,7 +107,7 @@ void handleCanUpgrade(final RestChannel channel, final RestRequest request, fina final var differencesList = new ArrayList>>(); for (final var configuration : configurations) { differencesList.add(computeDifferenceToUpdate(configuration) - .map(differences -> ValidationResult.success(new Tuple(configuration, differences.deepCopy())))); + .map(differences -> ValidationResult.success(new Tuple(configuration, differences)))); } return ValidationResult.combine(differencesList); })) @@ -166,11 +166,11 @@ private JsonNode filterRemoveOperations(final JsonNode diff) { final ArrayNode filteredDiff = JsonNodeFactory.instance.arrayNode(); diff.forEach(node -> { if (!isRemoveOperation(node)) { - filteredDiff.add(node.deepCopy()); + filteredDiff.add(node); return; } else { if (!hasRootLevelPath(node)) { - filteredDiff.add(node.deepCopy()); + filteredDiff.add(node); } } }); From 5db53a99e98bc6696d05a3c1b51a6436d37adb79 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 5 Mar 2024 09:28:01 -0600 Subject: [PATCH 09/22] Upgrade with tests Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 9 +- .../dlic/rest/api/ConfigUpgradeApiAction.java | 127 +++++++++++++----- .../rest/validation/ValidationResult.java | 34 +++-- 3 files changed, 125 insertions(+), 45 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index b254e3182a..c2fec40fd9 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -87,8 +87,7 @@ public void securityRolesUgrade() throws Exception { Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); final var defaultRolesResponse = client.get("_plugins/_security/api/roles/"); - final var roles = defaultRolesResponse.getBodyAs(JsonNode.class); - final var rolesCount = extractFieldNames(roles).size(); + final var rolesNames = extractFieldNames(defaultRolesResponse.getBodyAs(JsonNode.class)); final var checkForUpgrade = client.get("_plugins/_security/api/_upgrade_check"); System.out.println("checkForUpgrade Response: " + checkForUpgrade.getBody()); @@ -114,6 +113,12 @@ public void securityRolesUgrade() throws Exception { final var checkForUpgrade2 = client.get("_plugins/_security/api/_upgrade_check"); System.out.println("checkForUpgrade2 Response: " + checkForUpgrade2.getBody()); + final var upgradeResponse = client.post("_plugins/_security/api/_upgrade_perform"); + System.out.println("upgrade Response: " + upgradeResponse.getBody()); + + final var afterUpgradeRolesResponse = client.get("_plugins/_security/api/roles/"); + final var afterUpgradeRolesNames = extractFieldNames(defaultRolesResponse.getBodyAs(JsonNode.class)); + assertThat(afterUpgradeRolesNames, equalTo(rolesNames)); } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 6b81876af0..44b4a1656f 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -24,6 +24,7 @@ import java.security.PrivilegedExceptionAction; import java.util.ArrayList; import java.util.EnumSet; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -42,6 +43,7 @@ import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; import org.opensearch.security.configuration.ConfigurationRepository; +import org.opensearch.security.dlic.rest.api.RolesApiAction.RoleRequestContentValidator; import org.opensearch.security.dlic.rest.support.Utils; import org.opensearch.security.dlic.rest.validation.EndpointValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; @@ -58,6 +60,9 @@ import com.flipkart.zjsonpatch.DiffFlags; import com.flipkart.zjsonpatch.JsonDiff; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; + import org.opensearch.common.inject.Inject; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.rest.RestStatus; @@ -85,9 +90,12 @@ public class ConfigUpgradeApiAction extends AbstractApiAction { private final static Logger LOGGER = LogManager.getLogger(ConfigUpgradeApiAction.class); + private final static Set SUPPORTED_CTYPES = ImmutableSet.of(CType.ROLES); + private static final List routes = addRoutesPrefix(ImmutableList.of( new Route(Method.GET, "/_upgrade_check"), - new Route(Method.POST, "/_upgrade_perform"))); + new Route(Method.POST, "/_upgrade_perform") + )); @Inject public ConfigUpgradeApiAction( @@ -103,14 +111,7 @@ public ConfigUpgradeApiAction( void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { withIOException(() -> getAndValidateConfigurationsToUpgrade(request) - .map(configurations -> { - final var differencesList = new ArrayList>>(); - for (final var configuration : configurations) { - differencesList.add(computeDifferenceToUpdate(configuration) - .map(differences -> ValidationResult.success(new Tuple(configuration, differences)))); - } - return ValidationResult.combine(differencesList); - })) + .map(this::configurationDifferences)) .valid(differencesList -> { final var canUpgrade = differencesList.stream().anyMatch(entry -> entry.v2().size() > 0); @@ -120,9 +121,9 @@ void handleCanUpgrade(final RestChannel channel, final RestRequest request, fina if (canUpgrade) { final ObjectNode differences = JsonNodeFactory.instance.objectNode(); differencesList.forEach(t -> { - differences.put(t.v1().toLCString(), t.v2()); + differences.set(t.v1().toLCString(), t.v2()); }); - response.put("differences", differences); + response.set("differences", differences); } channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); }) @@ -130,15 +131,57 @@ void handleCanUpgrade(final RestChannel channel, final RestRequest request, fina } private void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { - throw new UnsupportedOperationException("Unimplemented method 'handleUpgrade'"); + withIOException(() -> getConfigurations(request) + .map(this::configurationDifferences)) + .map(diffs -> applyDifferences(request, diffs)) + .valid(updatedResources -> { + ok(channel, "Applied all differences: " + updatedResources); + }) + .error((status, toXContent) -> response(channel, status, toXContent)); + } + + ValidationResult>>>> applyDifferences(final RestRequest request, final List> differencesToUpdate) throws IOException { + final var updatedResources = new ArrayList>>>>(); + for (final Tuple difference : differencesToUpdate) { + updatedResources.add(loadConfiguration(difference.v1(), false, false) + .map(configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)) + .map(patchResults -> { + final var items = new HashMap(); + difference.v2().forEach(node -> { + final var item = pathRoot(node); + final var operation = node.get("op").asText(); + if (items.containsKey(item) && !items.get(item).equals(operation)) { + items.put(item, "modified"); + } else { + items.put(item, operation); + } + }); + + final var itemsGroupedByOperation = items.entrySet().stream().collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList()))); + + return ValidationResult.success(new Tuple<>(difference.v1(), itemsGroupedByOperation)); + }) + ) + ); + } + + return ValidationResult.merge(updatedResources); + } + + private ValidationResult>> configurationDifferences(final Set configurations) throws IOException { + final var differences = new ArrayList>>(); + for (final var configuration : configurations) { + differences.add(computeDifferenceToUpdate(configuration)); + } + return ValidationResult.merge(differences); } - private ValidationResult computeDifferenceToUpdate(final CType configType) throws IOException { + private ValidationResult> computeDifferenceToUpdate(final CType configType) throws IOException { return loadConfiguration(configType, false, false).map(activeRoles -> { final var activeRolesJson = Utils.convertJsonToJackson(activeRoles, false); final var defaultRolesJson = loadConfigFileAsJson(configType); final var rawDiff = JsonDiff.asJson(activeRolesJson, defaultRolesJson, EnumSet.of(DiffFlags.OMIT_VALUE_ON_REMOVE)); - return ValidationResult.success(filterRemoveOperations(rawDiff)); + return ValidationResult.success(new Tuple<>(configType, filterRemoveOperations(rawDiff))); }); } @@ -147,22 +190,18 @@ private ValidationResult> getAndValidateConfigurationsToUpgrade(final final var configurations = Optional.ofNullable(configs) .map(CType::fromStringValues) - .orElse(supportedConfigs()); + .orElse(SUPPORTED_CTYPES); - if (!configurations.stream().allMatch(supportedConfigs()::contains)) { + if (!configurations.stream().allMatch(SUPPORTED_CTYPES::contains)) { // Remove all supported configurations - configurations.removeAll(supportedConfigs()); + configurations.removeAll(SUPPORTED_CTYPES); return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Unsupported configurations for upgrade" + configurations)); } return ValidationResult.success(configurations); } - private Set supportedConfigs() { - return Set.of(CType.ROLES); - } - - private JsonNode filterRemoveOperations(final JsonNode diff) { + JsonNode filterRemoveOperations(final JsonNode diff) { final ArrayNode filteredDiff = JsonNodeFactory.instance.arrayNode(); diff.forEach(node -> { if (!isRemoveOperation(node)) { @@ -177,6 +216,10 @@ private JsonNode filterRemoveOperations(final JsonNode diff) { return filteredDiff; } + private String pathRoot(final JsonNode node) { + return node.get("path").asText().split("/")[1]; + } + private boolean hasRootLevelPath(final JsonNode node) { final var jsonPath = node.get("path").asText(); return jsonPath.charAt(0) == '/' && !jsonPath.substring(1).contains("/"); @@ -207,7 +250,7 @@ public List routes() { @Override protected CType getConfigType() { - return CType.ROLES; + throw new UnsupportedOperationException("This class supports multiple configuration types"); } @Override @@ -226,18 +269,7 @@ public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() { @Override public RequestContentValidator createRequestContentValidator(final Object... params) { - return RequestContentValidator.of(new RequestContentValidator.ValidationContext() { - - @Override - public Set mandatoryKeys() { - return Set.of("configs"); - } - - @Override - public Map allowedKeys() { - return Map.of("configs", DataType.ARRAY); - } - + return new RoleRequestContentValidator(new RequestContentValidator.ValidationContext() { @Override public Object[] params() { return params; @@ -247,8 +279,33 @@ public Object[] params() { public Settings settings() { return securityApiDependencies.settings(); } + + @Override + public Map allowedKeys() { + return Map.of("configs", DataType.ARRAY); + } }); } }; } + + public static class ConfigUpgradeContentValidator extends RequestContentValidator { + + protected ConfigUpgradeContentValidator(final ValidationContext validationContext) { + super(validationContext); + } + + @Override + public ValidationResult validate(RestRequest request) throws IOException { + return super.validate(request); + } + + @Override + public ValidationResult validate(RestRequest request, JsonNode jsonContent) throws IOException { + return super.validate(request, jsonContent); + } + + + } + } diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java index b0d2bf1a73..14c9d1546d 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java @@ -11,16 +11,25 @@ package org.opensearch.security.dlic.rest.validation; +import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; + import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Objects; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.opensearch.common.CheckedBiConsumer; import org.opensearch.common.CheckedConsumer; import org.opensearch.common.CheckedFunction; +import org.opensearch.common.collect.Tuple; import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.security.securityconf.impl.CType; + +import com.fasterxml.jackson.databind.JsonNode; public class ValidationResult { @@ -52,15 +61,19 @@ public static ValidationResult error(final RestStatus status, final ToXCo return new ValidationResult<>(status, errorMessage); } - public static ValidationResult> combine(final List> entries) { - final var returnList = new ArrayList(); - for (final var entry : entries) { - if (!entry.isValid()) { - return error(entry.status(), entry.errorMessage()); - } - returnList.add(entry.content); + /** + * Transforms a list of validation results into a single validation result of that lists contents. + * If any of the validation results are not valid, the first is returned as the error. + */ + public static ValidationResult> merge(final List> results) { + if (results.stream().allMatch(ValidationResult::isValid)) { + return success(results.stream().map(result -> result.content).collect(Collectors.toList())); } - return success(returnList); + + return results.stream().filter(result -> !result.isValid()) + .map(failedResult -> new ValidationResult>(failedResult.status, failedResult.errorMessage)) + .findFirst() + .get(); } public ValidationResult map(final CheckedFunction, IOException> mapper) throws IOException { @@ -99,4 +112,9 @@ public ToXContent errorMessage() { public C getContent() { return content; } + + public ValidationResult>> map(Object mapper) { + // TODO Auto-generated method stub + throw new UnsupportedOperationException("Unimplemented method 'map'"); + } } From 1c0f623c16fd3104dbfd8d9dc975877b3e6713d2 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 5 Mar 2024 09:31:00 -0600 Subject: [PATCH 10/22] Spotless formatting Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 39 ++--- .../dlic/rest/api/ConfigUpgradeApiAction.java | 160 ++++++++---------- .../rest/validation/ValidationResult.java | 12 +- 3 files changed, 89 insertions(+), 122 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index c2fec40fd9..73526db379 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -14,29 +14,22 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.stream.Stream; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; import com.fasterxml.jackson.databind.JsonNode; - import org.apache.commons.io.FileUtils; import org.awaitility.Awaitility; import org.junit.AfterClass; import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; + import org.opensearch.test.framework.TestSecurityConfig.User; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; -import org.opensearch.test.framework.cluster.TestRestClient; -import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.aMapWithSize; -import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasKey; @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) @@ -68,18 +61,17 @@ public static void cleanConfigurationDirectory() throws IOException { // @Test // public void shouldLoadDefaultConfiguration() { - // try (TestRestClient client = cluster.getRestClient(NEW_USER)) { - // Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); - // } - // try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { - // client.confirmCorrectCredentials(ADMIN_USER.getName()); - // HttpResponse response = client.get("_plugins/_security/api/internalusers"); - // response.assertStatusCode(200); - // Map users = response.getBodyAs(Map.class); - // assertThat(users, allOf(aMapWithSize(3), hasKey(ADMIN_USER.getName()), hasKey(NEW_USER.getName()), hasKey(LIMITED_USER.getName()))); - // } + // try (TestRestClient client = cluster.getRestClient(NEW_USER)) { + // Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); + // } + // try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + // client.confirmCorrectCredentials(ADMIN_USER.getName()); + // HttpResponse response = client.get("_plugins/_security/api/internalusers"); + // response.assertStatusCode(200); + // Map users = response.getBodyAs(Map.class); + // assertThat(users, allOf(aMapWithSize(3), hasKey(ADMIN_USER.getName()), hasKey(NEW_USER.getName()), hasKey(LIMITED_USER.getName()))); + // } // } - @Test public void securityRolesUgrade() throws Exception { @@ -92,7 +84,6 @@ public void securityRolesUgrade() throws Exception { final var checkForUpgrade = client.get("_plugins/_security/api/_upgrade_check"); System.out.println("checkForUpgrade Response: " + checkForUpgrade.getBody()); - final var roleToDelete = "flow_framework_full_access"; final var deleteRoleResponse = client.delete("_plugins/_security/api/roles/" + roleToDelete); deleteRoleResponse.assertStatusCode(200); @@ -103,12 +94,12 @@ public void securityRolesUgrade() throws Exception { final var roleToAlter = "flow_framework_read_access"; final String patchBody = "[{ \"op\": \"replace\", \"path\": \"/cluster_permissions\", \"value\":" + "[\"a\",\"b\",\"c\"]" - + "},{ \"op\": \"add\", \"path\": \"/index_permissions\", \"value\":" + + "},{ \"op\": \"add\", \"path\": \"/index_permissions\", \"value\":" + "[{\"index_patterns\":[\"*\"],\"allowed_actions\":[\"*\"]}]" - + "}]"; + + "}]"; final var updateRoleResponse = client.patch("_plugins/_security/api/roles/" + roleToAlter, patchBody); updateRoleResponse.assertStatusCode(200); - System.out.println("Updated Role Response: " +updateRoleResponse.getBody()); + System.out.println("Updated Role Response: " + updateRoleResponse.getBody()); final var checkForUpgrade2 = client.get("_plugins/_security/api/_upgrade_check"); System.out.println("checkForUpgrade2 Response: " + checkForUpgrade2.getBody()); @@ -125,6 +116,6 @@ public void securityRolesUgrade() throws Exception { private List extractFieldNames(final JsonNode json) { final var list = new ArrayList(); json.fieldNames().forEachRemaining(list::add); - return list; + return list; } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 44b4a1656f..cfa6e36a31 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -11,12 +11,6 @@ package org.opensearch.security.dlic.rest.api; -import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; -import static org.opensearch.security.dlic.rest.api.Responses.ok; -import static org.opensearch.security.dlic.rest.api.Responses.response; -import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; -import static org.opensearch.security.dlic.rest.support.Utils.withIOException; - import java.io.IOException; import java.nio.file.Path; import java.security.AccessController; @@ -31,14 +25,23 @@ import java.util.Set; import java.util.stream.Collectors; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; + import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.collect.Tuple; import org.opensearch.common.inject.Inject; import org.opensearch.common.settings.Settings; +import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.rest.RestStatus; +import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; @@ -53,38 +56,14 @@ import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; -import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.JsonNodeFactory; import com.flipkart.zjsonpatch.DiffFlags; import com.flipkart.zjsonpatch.JsonDiff; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; - -import org.opensearch.common.inject.Inject; -import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.rest.RestStatus; -import org.opensearch.rest.BytesRestResponse; -import org.opensearch.rest.RestChannel; -import org.opensearch.rest.RestRequest; -import org.opensearch.rest.RestRequest.Method; -import org.opensearch.security.configuration.ConfigurationRepository; -import org.opensearch.security.dlic.rest.support.Utils; -import org.opensearch.security.dlic.rest.validation.ValidationResult; -import org.opensearch.security.securityconf.impl.CType; -import org.opensearch.security.support.ConfigHelper; -import org.opensearch.threadpool.ThreadPool; - -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.JsonNodeFactory; -import com.fasterxml.jackson.databind.node.ObjectNode; -import com.flipkart.zjsonpatch.DiffFlags; -import com.flipkart.zjsonpatch.JsonDiff; -import com.google.common.collect.ImmutableList; +import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; +import static org.opensearch.security.dlic.rest.api.Responses.ok; +import static org.opensearch.security.dlic.rest.api.Responses.response; +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; +import static org.opensearch.security.dlic.rest.support.Utils.withIOException; public class ConfigUpgradeApiAction extends AbstractApiAction { @@ -92,10 +71,9 @@ public class ConfigUpgradeApiAction extends AbstractApiAction { private final static Set SUPPORTED_CTYPES = ImmutableSet.of(CType.ROLES); - private static final List routes = addRoutesPrefix(ImmutableList.of( - new Route(Method.GET, "/_upgrade_check"), - new Route(Method.POST, "/_upgrade_perform") - )); + private static final List routes = addRoutesPrefix( + ImmutableList.of(new Route(Method.GET, "/_upgrade_check"), new Route(Method.POST, "/_upgrade_perform")) + ); @Inject public ConfigUpgradeApiAction( @@ -110,64 +88,66 @@ public ConfigUpgradeApiAction( } void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { - withIOException(() -> getAndValidateConfigurationsToUpgrade(request) - .map(this::configurationDifferences)) - .valid(differencesList -> { - final var canUpgrade = differencesList.stream().anyMatch(entry -> entry.v2().size() > 0); - - final ObjectNode response = JsonNodeFactory.instance.objectNode(); - response.put("can_upgrade", canUpgrade); - - if (canUpgrade) { - final ObjectNode differences = JsonNodeFactory.instance.objectNode(); - differencesList.forEach(t -> { - differences.set(t.v1().toLCString(), t.v2()); - }); - response.set("differences", differences); - } - channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); - }) - .error((status, toXContent) -> response(channel, status, toXContent)); + withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).valid(differencesList -> { + final var canUpgrade = differencesList.stream().anyMatch(entry -> entry.v2().size() > 0); + + final ObjectNode response = JsonNodeFactory.instance.objectNode(); + response.put("can_upgrade", canUpgrade); + + if (canUpgrade) { + final ObjectNode differences = JsonNodeFactory.instance.objectNode(); + differencesList.forEach(t -> { differences.set(t.v1().toLCString(), t.v2()); }); + response.set("differences", differences); + } + channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); + }).error((status, toXContent) -> response(channel, status, toXContent)); } private void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { - withIOException(() -> getConfigurations(request) - .map(this::configurationDifferences)) - .map(diffs -> applyDifferences(request, diffs)) - .valid(updatedResources -> { - ok(channel, "Applied all differences: " + updatedResources); - }) - .error((status, toXContent) -> response(channel, status, toXContent)); + withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).map( + diffs -> applyDifferences(request, diffs) + ).valid(updatedResources -> { + ok(channel, "Applied all differences: " + updatedResources); + }).error((status, toXContent) -> response(channel, status, toXContent)); } - ValidationResult>>>> applyDifferences(final RestRequest request, final List> differencesToUpdate) throws IOException { + ValidationResult>>>> applyDifferences( + final RestRequest request, + final List> differencesToUpdate + ) throws IOException { final var updatedResources = new ArrayList>>>>(); for (final Tuple difference : differencesToUpdate) { - updatedResources.add(loadConfiguration(difference.v1(), false, false) - .map(configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)) - .map(patchResults -> { - final var items = new HashMap(); - difference.v2().forEach(node -> { - final var item = pathRoot(node); - final var operation = node.get("op").asText(); - if (items.containsKey(item) && !items.get(item).equals(operation)) { - items.put(item, "modified"); - } else { - items.put(item, operation); - } - }); - - final var itemsGroupedByOperation = items.entrySet().stream().collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList()))); - - return ValidationResult.success(new Tuple<>(difference.v1(), itemsGroupedByOperation)); - }) + updatedResources.add( + loadConfiguration(difference.v1(), false, false).map( + configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)).map( + patchResults -> { + final var items = new HashMap(); + difference.v2().forEach(node -> { + final var item = pathRoot(node); + final var operation = node.get("op").asText(); + if (items.containsKey(item) && !items.get(item).equals(operation)) { + items.put(item, "modified"); + } else { + items.put(item, operation); + } + }); + + final var itemsGroupedByOperation = items.entrySet() + .stream() + .collect( + Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList())) + ); + + return ValidationResult.success(new Tuple<>(difference.v1(), itemsGroupedByOperation)); + } + ) ) ); } return ValidationResult.merge(updatedResources); } - + private ValidationResult>> configurationDifferences(final Set configurations) throws IOException { final var differences = new ArrayList>>(); for (final var configuration : configurations) { @@ -187,15 +167,16 @@ private ValidationResult> computeDifferenceToUpdate(final private ValidationResult> getAndValidateConfigurationsToUpgrade(final RestRequest request) { final String[] configs = request.paramAsStringArray("configs", null); - - final var configurations = Optional.ofNullable(configs) - .map(CType::fromStringValues) - .orElse(SUPPORTED_CTYPES); + + final var configurations = Optional.ofNullable(configs).map(CType::fromStringValues).orElse(SUPPORTED_CTYPES); if (!configurations.stream().allMatch(SUPPORTED_CTYPES::contains)) { // Remove all supported configurations configurations.removeAll(SUPPORTED_CTYPES); - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Unsupported configurations for upgrade" + configurations)); + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage("Unsupported configurations for upgrade" + configurations) + ); } return ValidationResult.success(configurations); @@ -305,7 +286,6 @@ public ValidationResult validate(RestRequest request, JsonNode jsonCon return super.validate(request, jsonContent); } - } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java index 14c9d1546d..41db84fef0 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java @@ -11,15 +11,12 @@ package org.opensearch.security.dlic.rest.validation; -import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; - import java.io.IOException; -import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.Objects; import java.util.stream.Collectors; -import java.util.stream.Stream; + +import com.fasterxml.jackson.databind.JsonNode; import org.opensearch.common.CheckedBiConsumer; import org.opensearch.common.CheckedConsumer; @@ -29,8 +26,6 @@ import org.opensearch.core.xcontent.ToXContent; import org.opensearch.security.securityconf.impl.CType; -import com.fasterxml.jackson.databind.JsonNode; - public class ValidationResult { private final RestStatus status; @@ -70,7 +65,8 @@ public static ValidationResult> merge(final List return success(results.stream().map(result -> result.content).collect(Collectors.toList())); } - return results.stream().filter(result -> !result.isValid()) + return results.stream() + .filter(result -> !result.isValid()) .map(failedResult -> new ValidationResult>(failedResult.status, failedResult.errorMessage)) .findFirst() .get(); From 051ed7eddb66f49548b35c86881b9dda4fd9d6b9 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 02:04:27 +0000 Subject: [PATCH 11/22] Add more stuff Signed-off-by: Peter Nied --- .../dlic/rest/api/ConfigUpgradeApiAction.java | 164 ++++++++++++------ .../validation/RequestContentValidator.java | 4 +- 2 files changed, 111 insertions(+), 57 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index cfa6e36a31..895580d776 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -11,6 +11,11 @@ package org.opensearch.security.dlic.rest.api; +import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; +import static org.opensearch.security.dlic.rest.api.Responses.response; +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; +import static org.opensearch.security.dlic.rest.support.Utils.withIOException; + import java.io.IOException; import java.nio.file.Path; import java.security.AccessController; @@ -25,15 +30,8 @@ import java.util.Set; import java.util.stream.Collectors; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.JsonNodeFactory; -import com.fasterxml.jackson.databind.node.ObjectNode; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; - import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.collect.Tuple; @@ -46,7 +44,6 @@ import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; import org.opensearch.security.configuration.ConfigurationRepository; -import org.opensearch.security.dlic.rest.api.RolesApiAction.RoleRequestContentValidator; import org.opensearch.security.dlic.rest.support.Utils; import org.opensearch.security.dlic.rest.validation.EndpointValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; @@ -56,14 +53,14 @@ import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.flipkart.zjsonpatch.DiffFlags; import com.flipkart.zjsonpatch.JsonDiff; - -import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; -import static org.opensearch.security.dlic.rest.api.Responses.ok; -import static org.opensearch.security.dlic.rest.api.Responses.response; -import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; -import static org.opensearch.security.dlic.rest.support.Utils.withIOException; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; public class ConfigUpgradeApiAction extends AbstractApiAction { @@ -71,6 +68,8 @@ public class ConfigUpgradeApiAction extends AbstractApiAction { private final static Set SUPPORTED_CTYPES = ImmutableSet.of(CType.ROLES); + private final static String REQUEST_PARAM_CONFIGS_KEY = "configs"; + private static final List routes = addRoutesPrefix( ImmutableList.of(new Route(Method.GET, "/_upgrade_check"), new Route(Method.POST, "/_upgrade_perform")) ); @@ -89,56 +88,51 @@ public ConfigUpgradeApiAction( void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).valid(differencesList -> { - final var canUpgrade = differencesList.stream().anyMatch(entry -> entry.v2().size() > 0); + final var allConfigItemChanges = differencesList.stream().map(kvp -> new ConfigItemChanges(kvp.v1(), classifyChanges(kvp.v2()))).collect(Collectors.toList()); + + final var upgradeAvaliable = allConfigItemChanges.stream().anyMatch(ConfigItemChanges::hasChanges); final ObjectNode response = JsonNodeFactory.instance.objectNode(); - response.put("can_upgrade", canUpgrade); + response.put("upgradeAvaliable", upgradeAvaliable); - if (canUpgrade) { + if (upgradeAvaliable) { final ObjectNode differences = JsonNodeFactory.instance.objectNode(); - differencesList.forEach(t -> { differences.set(t.v1().toLCString(), t.v2()); }); - response.set("differences", differences); + allConfigItemChanges.forEach(configItemChanges -> configItemChanges.addToNode(differences)); + response.set("upgradeActions", differences); } channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); }).error((status, toXContent) -> response(channel, status, toXContent)); } private void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { - withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).map( + withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)) + .map(this::verifyHasDifferences) + .map( diffs -> applyDifferences(request, diffs) - ).valid(updatedResources -> { - ok(channel, "Applied all differences: " + updatedResources); + ).valid(updatedConfigs -> { + final var response = JsonNodeFactory.instance.objectNode(); + response.put("status", "OK"); + + final var allUpdates = JsonNodeFactory.instance.objectNode(); + updatedConfigs.forEach(configItemChanges -> configItemChanges.addToNode(allUpdates)); + response.set("upgrades", allUpdates); + + channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); }).error((status, toXContent) -> response(channel, status, toXContent)); } - ValidationResult>>>> applyDifferences( + ValidationResult> applyDifferences( final RestRequest request, final List> differencesToUpdate ) throws IOException { - final var updatedResources = new ArrayList>>>>(); + final var updatedResources = new ArrayList>(); for (final Tuple difference : differencesToUpdate) { updatedResources.add( loadConfiguration(difference.v1(), false, false).map( configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)).map( patchResults -> { - final var items = new HashMap(); - difference.v2().forEach(node -> { - final var item = pathRoot(node); - final var operation = node.get("op").asText(); - if (items.containsKey(item) && !items.get(item).equals(operation)) { - items.put(item, "modified"); - } else { - items.put(item, operation); - } - }); - - final var itemsGroupedByOperation = items.entrySet() - .stream() - .collect( - Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList())) - ); - - return ValidationResult.success(new Tuple<>(difference.v1(), itemsGroupedByOperation)); + final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), classifyChanges(difference.v2())); + return ValidationResult.success(itemsGroupedByOperation); } ) ) @@ -148,6 +142,70 @@ ValidationResult>>>> applyDifferences return ValidationResult.merge(updatedResources); } + /** Add syntaxic sugar for interacting with config changes */ + static class ConfigItemChanges { + + private final CType config; + private final Map> itemsGroupedByOperation; + + public ConfigItemChanges(final CType config, final Map> itemsGroupedByOperation) { + this.config = config; + this.itemsGroupedByOperation = itemsGroupedByOperation; + } + + public boolean hasChanges() { + return !itemsGroupedByOperation.isEmpty(); + } + + public void addToNode(final ObjectNode node) { + final var allOperations = JsonNodeFactory.instance.objectNode(); + itemsGroupedByOperation.forEach((operation, items) -> { + final var arrayNode = allOperations.putArray(operation); + items.forEach(arrayNode::add); + }); + node.set(config.toLCString(), allOperations); + } + } + + private static Map> classifyChanges(final JsonNode differences){ + final var items = new HashMap(); + differences.forEach(node -> { + final var item = pathRoot(node); + final var operation = node.get("op").asText(); + if (items.containsKey(item) && !items.get(item).equals(operation)) { + items.put(item, "modify"); + } else { + items.put(item, operation); + } + }); + + final var itemsGroupedByOperation = items.entrySet() + .stream() + .collect( + Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList())) + ); + return itemsGroupedByOperation; + } + + private ValidationResult>> verifyHasDifferences(List> diffs) { + if (diffs.isEmpty()) { + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage("Unable to upgrade, no differences found") + ); + } + + for (final var diff : diffs) { + if (diff.v2().size() == 0) { + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage("Unable to upgrade, no differences found in '" + diff.v1().toLCString() + "' config") + ); + } + } + return ValidationResult.success(diffs); + } + private ValidationResult>> configurationDifferences(final Set configurations) throws IOException { final var differences = new ArrayList>>(); for (final var configuration : configurations) { @@ -166,7 +224,7 @@ private ValidationResult> computeDifferenceToUpdate(final } private ValidationResult> getAndValidateConfigurationsToUpgrade(final RestRequest request) { - final String[] configs = request.paramAsStringArray("configs", null); + final String[] configs = request.paramAsStringArray(REQUEST_PARAM_CONFIGS_KEY, null); final var configurations = Optional.ofNullable(configs).map(CType::fromStringValues).orElse(SUPPORTED_CTYPES); @@ -197,16 +255,16 @@ JsonNode filterRemoveOperations(final JsonNode diff) { return filteredDiff; } - private String pathRoot(final JsonNode node) { + private static String pathRoot(final JsonNode node) { return node.get("path").asText().split("/")[1]; } - private boolean hasRootLevelPath(final JsonNode node) { + private static boolean hasRootLevelPath(final JsonNode node) { final var jsonPath = node.get("path").asText(); return jsonPath.charAt(0) == '/' && !jsonPath.substring(1).contains("/"); } - private boolean isRemoveOperation(final JsonNode node) { + private static boolean isRemoveOperation(final JsonNode node) { return node.get("op").asText().equals("remove"); } @@ -250,7 +308,7 @@ public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() { @Override public RequestContentValidator createRequestContentValidator(final Object... params) { - return new RoleRequestContentValidator(new RequestContentValidator.ValidationContext() { + return new ConfigUpgradeContentValidator(new RequestContentValidator.ValidationContext() { @Override public Object[] params() { return params; @@ -263,13 +321,14 @@ public Settings settings() { @Override public Map allowedKeys() { - return Map.of("configs", DataType.ARRAY); + return Map.of(REQUEST_PARAM_CONFIGS_KEY, DataType.ARRAY); } }); } }; } + /** More permissions validation that default ContentValidator */ public static class ConfigUpgradeContentValidator extends RequestContentValidator { protected ConfigUpgradeContentValidator(final ValidationContext validationContext) { @@ -277,13 +336,8 @@ protected ConfigUpgradeContentValidator(final ValidationContext validationContex } @Override - public ValidationResult validate(RestRequest request) throws IOException { - return super.validate(request); - } - - @Override - public ValidationResult validate(RestRequest request, JsonNode jsonContent) throws IOException { - return super.validate(request, jsonContent); + public ValidationResult validate(final RestRequest request, final JsonNode jsonContent) throws IOException { + return validateContentSize(jsonContent);//.map(this::validateJsonKeys); TODO use this on the request but disable on patching } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java index 452bdd72e4..4d9faf096c 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java @@ -142,7 +142,7 @@ private ValidationResult parseRequestContent(final RestRequest request } } - private ValidationResult validateContentSize(final JsonNode jsonContent) { + protected ValidationResult validateContentSize(final JsonNode jsonContent) { if (jsonContent.isEmpty()) { this.validationError = ValidationError.PAYLOAD_MANDATORY; return ValidationResult.error(RestStatus.BAD_REQUEST, this); @@ -150,7 +150,7 @@ private ValidationResult validateContentSize(final JsonNode jsonConten return ValidationResult.success(jsonContent); } - private ValidationResult validateJsonKeys(final JsonNode jsonContent) { + protected ValidationResult validateJsonKeys(final JsonNode jsonContent) { final Set requestedKeys = new HashSet<>(); jsonContent.fieldNames().forEachRemaining(requestedKeys::add); // mandatory settings, one of ... From a06413652df0421e7e3be9017f3f412f304453a0 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 02:05:18 +0000 Subject: [PATCH 12/22] Spotless Signed-off-by: Peter Nied --- .../dlic/rest/api/ConfigUpgradeApiAction.java | 50 +++++++++---------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 895580d776..a35dccb7b0 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -11,11 +11,6 @@ package org.opensearch.security.dlic.rest.api; -import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; -import static org.opensearch.security.dlic.rest.api.Responses.response; -import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; -import static org.opensearch.security.dlic.rest.support.Utils.withIOException; - import java.io.IOException; import java.nio.file.Path; import java.security.AccessController; @@ -30,8 +25,15 @@ import java.util.Set; import java.util.stream.Collectors; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; + import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.collect.Tuple; @@ -53,14 +55,13 @@ import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.JsonNodeFactory; -import com.fasterxml.jackson.databind.node.ObjectNode; import com.flipkart.zjsonpatch.DiffFlags; import com.flipkart.zjsonpatch.JsonDiff; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; + +import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; +import static org.opensearch.security.dlic.rest.api.Responses.response; +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; +import static org.opensearch.security.dlic.rest.support.Utils.withIOException; public class ConfigUpgradeApiAction extends AbstractApiAction { @@ -88,7 +89,9 @@ public ConfigUpgradeApiAction( void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).valid(differencesList -> { - final var allConfigItemChanges = differencesList.stream().map(kvp -> new ConfigItemChanges(kvp.v1(), classifyChanges(kvp.v2()))).collect(Collectors.toList()); + final var allConfigItemChanges = differencesList.stream() + .map(kvp -> new ConfigItemChanges(kvp.v1(), classifyChanges(kvp.v2()))) + .collect(Collectors.toList()); final var upgradeAvaliable = allConfigItemChanges.stream().anyMatch(ConfigItemChanges::hasChanges); @@ -104,12 +107,10 @@ void handleCanUpgrade(final RestChannel channel, final RestRequest request, fina }).error((status, toXContent) -> response(channel, status, toXContent)); } - private void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { - withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)) - .map(this::verifyHasDifferences) - .map( - diffs -> applyDifferences(request, diffs) - ).valid(updatedConfigs -> { + void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { + withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).map( + this::verifyHasDifferences + ).map(diffs -> applyDifferences(request, diffs)).valid(updatedConfigs -> { final var response = JsonNodeFactory.instance.objectNode(); response.put("status", "OK"); @@ -167,7 +168,7 @@ public void addToNode(final ObjectNode node) { } } - private static Map> classifyChanges(final JsonNode differences){ + private static Map> classifyChanges(final JsonNode differences) { final var items = new HashMap(); differences.forEach(node -> { final var item = pathRoot(node); @@ -181,18 +182,13 @@ private static Map> classifyChanges(final JsonNode differen final var itemsGroupedByOperation = items.entrySet() .stream() - .collect( - Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList())) - ); + .collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList()))); return itemsGroupedByOperation; } private ValidationResult>> verifyHasDifferences(List> diffs) { if (diffs.isEmpty()) { - return ValidationResult.error( - RestStatus.BAD_REQUEST, - badRequestMessage("Unable to upgrade, no differences found") - ); + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Unable to upgrade, no differences found")); } for (final var diff : diffs) { @@ -337,7 +333,7 @@ protected ConfigUpgradeContentValidator(final ValidationContext validationContex @Override public ValidationResult validate(final RestRequest request, final JsonNode jsonContent) throws IOException { - return validateContentSize(jsonContent);//.map(this::validateJsonKeys); TODO use this on the request but disable on patching + return validateContentSize(jsonContent);// .map(this::validateJsonKeys); TODO use this on the request but disable on patching } } From f92c510fb3f386054dc498286c30862d1e011e9b Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 02:26:25 +0000 Subject: [PATCH 13/22] Clean up integration test Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index 73526db379..f5b000188a 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -27,9 +27,14 @@ import org.opensearch.test.framework.TestSecurityConfig.User; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; +import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.aMapWithSize; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasKey; @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) @@ -59,19 +64,22 @@ public static void cleanConfigurationDirectory() throws IOException { FileUtils.deleteDirectory(configurationFolder.toFile()); } - // @Test - // public void shouldLoadDefaultConfiguration() { - // try (TestRestClient client = cluster.getRestClient(NEW_USER)) { - // Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); - // } - // try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { - // client.confirmCorrectCredentials(ADMIN_USER.getName()); - // HttpResponse response = client.get("_plugins/_security/api/internalusers"); - // response.assertStatusCode(200); - // Map users = response.getBodyAs(Map.class); - // assertThat(users, allOf(aMapWithSize(3), hasKey(ADMIN_USER.getName()), hasKey(NEW_USER.getName()), hasKey(LIMITED_USER.getName()))); - // } - // } + @Test + public void shouldLoadDefaultConfiguration() { + try (TestRestClient client = cluster.getRestClient(NEW_USER)) { + Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); + } + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + client.confirmCorrectCredentials(ADMIN_USER.getName()); + HttpResponse response = client.get("_plugins/_security/api/internalusers"); + response.assertStatusCode(200); + Map users = response.getBodyAs(Map.class); + assertThat( + users, + allOf(aMapWithSize(3), hasKey(ADMIN_USER.getName()), hasKey(NEW_USER.getName()), hasKey(LIMITED_USER.getName())) + ); + } + } @Test public void securityRolesUgrade() throws Exception { @@ -108,7 +116,7 @@ public void securityRolesUgrade() throws Exception { System.out.println("upgrade Response: " + upgradeResponse.getBody()); final var afterUpgradeRolesResponse = client.get("_plugins/_security/api/roles/"); - final var afterUpgradeRolesNames = extractFieldNames(defaultRolesResponse.getBodyAs(JsonNode.class)); + final var afterUpgradeRolesNames = extractFieldNames(afterUpgradeRolesResponse.getBodyAs(JsonNode.class)); assertThat(afterUpgradeRolesNames, equalTo(rolesNames)); } } From 30d6bfcc43df52e64e761d17909e636d8474c8e3 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 02:27:47 +0000 Subject: [PATCH 14/22] Clean up changes around ValidationResult Signed-off-by: Peter Nied --- .../dlic/rest/validation/ValidationResult.java | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java index 41db84fef0..921ed4675d 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/ValidationResult.java @@ -16,15 +16,11 @@ import java.util.Objects; import java.util.stream.Collectors; -import com.fasterxml.jackson.databind.JsonNode; - import org.opensearch.common.CheckedBiConsumer; import org.opensearch.common.CheckedConsumer; import org.opensearch.common.CheckedFunction; -import org.opensearch.common.collect.Tuple; import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.security.securityconf.impl.CType; public class ValidationResult { @@ -104,13 +100,4 @@ public boolean isValid() { public ToXContent errorMessage() { return errorMessage; } - - public C getContent() { - return content; - } - - public ValidationResult>> map(Object mapper) { - // TODO Auto-generated method stub - throw new UnsupportedOperationException("Unimplemented method 'map'"); - } } From 3a1e1ff33a937e2ede256e40bc8f835160c98874 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 02:52:05 +0000 Subject: [PATCH 15/22] Found out the configuration changes weren't actually pushed Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 2 +- .../dlic/rest/api/ConfigUpgradeApiAction.java | 138 ++++++++++-------- 2 files changed, 80 insertions(+), 60 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index f5b000188a..c6ceb51704 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -117,7 +117,7 @@ public void securityRolesUgrade() throws Exception { final var afterUpgradeRolesResponse = client.get("_plugins/_security/api/roles/"); final var afterUpgradeRolesNames = extractFieldNames(afterUpgradeRolesResponse.getBodyAs(JsonNode.class)); - assertThat(afterUpgradeRolesNames, equalTo(rolesNames)); + assertThat(afterUpgradeRolesResponse.getBody(), afterUpgradeRolesNames, equalTo(rolesNames)); } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index a35dccb7b0..56c2807d9f 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -11,6 +11,11 @@ package org.opensearch.security.dlic.rest.api; +import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; +import static org.opensearch.security.dlic.rest.api.Responses.response; +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; +import static org.opensearch.security.dlic.rest.support.Utils.withIOException; + import java.io.IOException; import java.nio.file.Path; import java.security.AccessController; @@ -25,27 +30,24 @@ import java.util.Set; import java.util.stream.Collectors; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.JsonNodeFactory; -import com.fasterxml.jackson.databind.node.ObjectNode; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; - +import org.opensearch.action.index.IndexResponse; import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.action.ActionFuture; import org.opensearch.common.collect.Tuple; import org.opensearch.common.inject.Inject; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.action.ActionListener; import org.opensearch.core.rest.RestStatus; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; import org.opensearch.security.configuration.ConfigurationRepository; +import org.opensearch.security.dlic.rest.api.ConfigUpgradeApiAction.ConfigItemChanges; import org.opensearch.security.dlic.rest.support.Utils; import org.opensearch.security.dlic.rest.validation.EndpointValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; @@ -55,13 +57,14 @@ import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.flipkart.zjsonpatch.DiffFlags; import com.flipkart.zjsonpatch.JsonDiff; - -import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; -import static org.opensearch.security.dlic.rest.api.Responses.response; -import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; -import static org.opensearch.security.dlic.rest.support.Utils.withIOException; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; public class ConfigUpgradeApiAction extends AbstractApiAction { @@ -90,7 +93,7 @@ public ConfigUpgradeApiAction( void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).valid(differencesList -> { final var allConfigItemChanges = differencesList.stream() - .map(kvp -> new ConfigItemChanges(kvp.v1(), classifyChanges(kvp.v2()))) + .map(kvp -> new ConfigItemChanges(kvp.v1(), kvp.v2())) .collect(Collectors.toList()); final var upgradeAvaliable = allConfigItemChanges.stream().anyMatch(ConfigItemChanges::hasChanges); @@ -124,15 +127,33 @@ void handleUpgrade(final RestChannel channel, final RestRequest request, final C ValidationResult> applyDifferences( final RestRequest request, + final Client client, final List> differencesToUpdate ) throws IOException { final var updatedResources = new ArrayList>(); for (final Tuple difference : differencesToUpdate) { updatedResources.add( loadConfiguration(difference.v1(), false, false).map( - configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)).map( + configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)) + .map(patchResults -> { + saveAndUpdateConfigs(securityApiDependencies.securityIndexName(), client, difference.v1(), configuration, new ActionListener<>(){ + + @Override + public void onResponse(IndexResponse response) { + // TODO: oh my - how did we get here + } + + @Override + public void onFailure(Exception e) { + } + }); + + }) + .map( patchResults -> { - final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), classifyChanges(difference.v2())); + + + final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), difference.v2()); return ValidationResult.success(itemsGroupedByOperation); } ) @@ -143,49 +164,6 @@ ValidationResult> applyDifferences( return ValidationResult.merge(updatedResources); } - /** Add syntaxic sugar for interacting with config changes */ - static class ConfigItemChanges { - - private final CType config; - private final Map> itemsGroupedByOperation; - - public ConfigItemChanges(final CType config, final Map> itemsGroupedByOperation) { - this.config = config; - this.itemsGroupedByOperation = itemsGroupedByOperation; - } - - public boolean hasChanges() { - return !itemsGroupedByOperation.isEmpty(); - } - - public void addToNode(final ObjectNode node) { - final var allOperations = JsonNodeFactory.instance.objectNode(); - itemsGroupedByOperation.forEach((operation, items) -> { - final var arrayNode = allOperations.putArray(operation); - items.forEach(arrayNode::add); - }); - node.set(config.toLCString(), allOperations); - } - } - - private static Map> classifyChanges(final JsonNode differences) { - final var items = new HashMap(); - differences.forEach(node -> { - final var item = pathRoot(node); - final var operation = node.get("op").asText(); - if (items.containsKey(item) && !items.get(item).equals(operation)) { - items.put(item, "modify"); - } else { - items.put(item, operation); - } - }); - - final var itemsGroupedByOperation = items.entrySet() - .stream() - .collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList()))); - return itemsGroupedByOperation; - } - private ValidationResult>> verifyHasDifferences(List> diffs) { if (diffs.isEmpty()) { return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Unable to upgrade, no differences found")); @@ -325,7 +303,7 @@ public Map allowedKeys() { } /** More permissions validation that default ContentValidator */ - public static class ConfigUpgradeContentValidator extends RequestContentValidator { + static class ConfigUpgradeContentValidator extends RequestContentValidator { protected ConfigUpgradeContentValidator(final ValidationContext validationContext) { super(validationContext); @@ -338,4 +316,46 @@ public ValidationResult validate(final RestRequest request, final Json } + /** Tranforms config changes from a raw PATCH into simplier view */ + static class ConfigItemChanges { + + private final CType config; + private final Map> itemsGroupedByOperation; + + public ConfigItemChanges(final CType config, final JsonNode differences) { + this.config = config; + this.itemsGroupedByOperation = classifyChanges(differences); + } + + public boolean hasChanges() { + return !itemsGroupedByOperation.isEmpty(); + } + + public void addToNode(final ObjectNode node) { + final var allOperations = JsonNodeFactory.instance.objectNode(); + itemsGroupedByOperation.forEach((operation, items) -> { + final var arrayNode = allOperations.putArray(operation); + items.forEach(arrayNode::add); + }); + node.set(config.toLCString(), allOperations); + } + + private static Map> classifyChanges(final JsonNode differences) { + final var items = new HashMap(); + differences.forEach(node -> { + final var item = pathRoot(node); + final var operation = node.get("op").asText(); + if (items.containsKey(item) && !items.get(item).equals(operation)) { + items.put(item, "modify"); + } else { + items.put(item, operation); + } + }); + + final var itemsGroupedByOperation = items.entrySet() + .stream() + .collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList()))); + return itemsGroupedByOperation; + } + } } From 0a77c175e3fa4c60f41ea5c73307246c4fbb436d Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 19:02:08 +0000 Subject: [PATCH 16/22] Make sure upgraded configs are commited Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 11 ++-- .../dlic/rest/api/AbstractApiAction.java | 50 ++++++++++----- .../dlic/rest/api/ConfigUpgradeApiAction.java | 64 ++++++++----------- 3 files changed, 67 insertions(+), 58 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index c6ceb51704..fe6bf2d7a3 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -11,9 +11,10 @@ import java.io.IOException; import java.nio.file.Path; -import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; import com.fasterxml.jackson.databind.JsonNode; @@ -121,9 +122,9 @@ public void securityRolesUgrade() throws Exception { } } - private List extractFieldNames(final JsonNode json) { - final var list = new ArrayList(); - json.fieldNames().forEachRemaining(list::add); - return list; + private Set extractFieldNames(final JsonNode json) { + final var set = new HashSet(); + json.fieldNames().forEachRemaining(set::add); + return set; } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java index ef8a00d700..e53a53715d 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java @@ -35,13 +35,16 @@ import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.CheckedSupplier; +import org.opensearch.common.action.ActionFuture; import org.opensearch.common.util.concurrent.ThreadContext.StoredContext; -import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.Strings; +import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.common.transport.TransportAddress; import org.opensearch.core.rest.RestStatus; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentHelper; import org.opensearch.index.engine.VersionConflictEngineException; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; @@ -336,7 +339,7 @@ final void saveOrUpdateConfiguration( final SecurityDynamicConfiguration configuration, final OnSucessActionListener onSucessActionListener ) { - saveAndUpdateConfigs(securityApiDependencies.securityIndexName(), client, getConfigType(), configuration, onSucessActionListener); + saveAndUpdateConfigsAsync(securityApiDependencies, client, getConfigType(), configuration, onSucessActionListener); } protected final String nameParam(final RestRequest request) { @@ -485,30 +488,45 @@ public final void onFailure(Exception e) { } - public static void saveAndUpdateConfigs( - final String indexName, + public static ActionFuture saveAndUpdateConfigs( + final SecurityApiDependencies dependencies, + final Client client, + final CType cType, + final SecurityDynamicConfiguration configuration + ) { + final var request = createIndexRequestForConfig(dependencies, cType, configuration); + return client.index(request); + } + + public static void saveAndUpdateConfigsAsync( + final SecurityApiDependencies dependencies, final Client client, final CType cType, final SecurityDynamicConfiguration configuration, final ActionListener actionListener ) { - final IndexRequest ir = new IndexRequest(indexName); - final String id = cType.toLCString(); + final var ir = createIndexRequestForConfig(dependencies, cType, configuration); + client.index(ir, new ConfigUpdatingActionListener<>(new String[] { cType.toLCString() }, client, actionListener)); + } + private static IndexRequest createIndexRequestForConfig( + final SecurityApiDependencies dependencies, + final CType cType, + final SecurityDynamicConfiguration configuration + ) { configuration.removeStatic(); - + final BytesReference content; try { - client.index( - ir.id(id) - .setRefreshPolicy(RefreshPolicy.IMMEDIATE) - .setIfSeqNo(configuration.getSeqNo()) - .setIfPrimaryTerm(configuration.getPrimaryTerm()) - .source(id, XContentHelper.toXContent(configuration, XContentType.JSON, false)), - new ConfigUpdatingActionListener<>(new String[] { id }, client, actionListener) - ); - } catch (IOException e) { + content = XContentHelper.toXContent(configuration, XContentType.JSON, ToXContent.EMPTY_PARAMS, false); + } catch (final IOException e) { throw ExceptionsHelper.convertToOpenSearchException(e); } + + return new IndexRequest(dependencies.securityIndexName()).id(cType.toLCString()) + .setRefreshPolicy(RefreshPolicy.IMMEDIATE) + .setIfSeqNo(configuration.getSeqNo()) + .setIfPrimaryTerm(configuration.getPrimaryTerm()) + .source(cType.toLCString(), content); } protected static class ConfigUpdatingActionListener implements ActionListener { diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 56c2807d9f..8e171e1e34 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -11,11 +11,6 @@ package org.opensearch.security.dlic.rest.api; -import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; -import static org.opensearch.security.dlic.rest.api.Responses.response; -import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; -import static org.opensearch.security.dlic.rest.support.Utils.withIOException; - import java.io.IOException; import java.nio.file.Path; import java.security.AccessController; @@ -30,17 +25,21 @@ import java.util.Set; import java.util.stream.Collectors; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.opensearch.action.index.IndexResponse; + import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.action.ActionFuture; import org.opensearch.common.collect.Tuple; import org.opensearch.common.inject.Inject; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.action.ActionListener; import org.opensearch.core.rest.RestStatus; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; @@ -57,14 +56,13 @@ import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.JsonNodeFactory; -import com.fasterxml.jackson.databind.node.ObjectNode; import com.flipkart.zjsonpatch.DiffFlags; import com.flipkart.zjsonpatch.JsonDiff; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; + +import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; +import static org.opensearch.security.dlic.rest.api.Responses.response; +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; +import static org.opensearch.security.dlic.rest.support.Utils.withIOException; public class ConfigUpgradeApiAction extends AbstractApiAction { @@ -113,7 +111,7 @@ void handleCanUpgrade(final RestChannel channel, final RestRequest request, fina void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).map( this::verifyHasDifferences - ).map(diffs -> applyDifferences(request, diffs)).valid(updatedConfigs -> { + ).map(diffs -> applyDifferences(request, client, diffs)).valid(updatedConfigs -> { final var response = JsonNodeFactory.instance.objectNode(); response.put("status", "OK"); @@ -134,29 +132,21 @@ ValidationResult> applyDifferences( for (final Tuple difference : differencesToUpdate) { updatedResources.add( loadConfiguration(difference.v1(), false, false).map( - configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)) - .map(patchResults -> { - saveAndUpdateConfigs(securityApiDependencies.securityIndexName(), client, difference.v1(), configuration, new ActionListener<>(){ - - @Override - public void onResponse(IndexResponse response) { - // TODO: oh my - how did we get here - } - - @Override - public void onFailure(Exception e) { - } - }); - - }) - .map( + configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)).map( patchResults -> { - - - final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), difference.v2()); - return ValidationResult.success(itemsGroupedByOperation); + final var response = saveAndUpdateConfigs( + securityApiDependencies, + client, + difference.v1(), + patchResults.configuration() + ); + return ValidationResult.success(response.actionGet()); } - ) + ).map(indexResponse -> { + + final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), difference.v2()); + return ValidationResult.success(itemsGroupedByOperation); + }) ) ); } @@ -351,7 +341,7 @@ private static Map> classifyChanges(final JsonNode differen items.put(item, operation); } }); - + final var itemsGroupedByOperation = items.entrySet() .stream() .collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList()))); From 7bec1f6b4015ae9e32344aac214d95976fbb4b21 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 8 Mar 2024 00:10:26 +0000 Subject: [PATCH 17/22] Tests Signed-off-by: Peter Nied --- .../dlic/rest/api/AbstractApiAction.java | 4 +- .../dlic/rest/api/ConfigUpgradeApiAction.java | 156 ++++++---- .../api/ConfigUpgradeApiActionUnitTest.java | 274 ++++++++++++++++++ 3 files changed, 376 insertions(+), 58 deletions(-) create mode 100644 src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java index e53a53715d..a38d618da0 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java @@ -229,7 +229,7 @@ protected final ValidationResult patchEntity( ); } - protected final ValidationResult patchEntities( + protected ValidationResult patchEntities( final RestRequest request, final JsonNode patchContent, final SecurityConfiguration securityConfiguration @@ -370,7 +370,7 @@ protected final ValidationResult loadConfiguration(final ); } - protected final ValidationResult> loadConfiguration( + protected ValidationResult> loadConfiguration( final CType cType, boolean omitSensitiveData, final boolean logComplianceEvent diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 8e171e1e34..0cb51e3bb5 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -12,6 +12,7 @@ package org.opensearch.security.dlic.rest.api; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.file.Path; import java.security.AccessController; import java.security.PrivilegedActionException; @@ -53,6 +54,7 @@ import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType; import org.opensearch.security.dlic.rest.validation.ValidationResult; import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; @@ -84,12 +86,12 @@ public ConfigUpgradeApiAction( ) { super(Endpoint.CONFIG, clusterService, threadPool, securityApiDependencies); this.requestHandlersBuilder.configureRequestHandlers(rhb -> { - rhb.allMethodsNotImplemented().add(Method.GET, this::handleCanUpgrade).add(Method.POST, this::handleUpgrade); + rhb.allMethodsNotImplemented().add(Method.GET, this::canUpgrade).add(Method.POST, this::performUpgrade); }); } - void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { - withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).valid(differencesList -> { + void canUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { + getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences).valid(differencesList -> { final var allConfigItemChanges = differencesList.stream() .map(kvp -> new ConfigItemChanges(kvp.v1(), kvp.v2())) .collect(Collectors.toList()); @@ -108,53 +110,64 @@ void handleCanUpgrade(final RestChannel channel, final RestRequest request, fina }).error((status, toXContent) -> response(channel, status, toXContent)); } - void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { - withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).map( - this::verifyHasDifferences - ).map(diffs -> applyDifferences(request, client, diffs)).valid(updatedConfigs -> { - final var response = JsonNodeFactory.instance.objectNode(); - response.put("status", "OK"); - - final var allUpdates = JsonNodeFactory.instance.objectNode(); - updatedConfigs.forEach(configItemChanges -> configItemChanges.addToNode(allUpdates)); - response.set("upgrades", allUpdates); - - channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); - }).error((status, toXContent) -> response(channel, status, toXContent)); + void performUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { + getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences) + .map(this::verifyHasDifferences) + .map(diffs -> applyDifferences(request, client, diffs)) + .valid(updatedConfigs -> { + final var response = JsonNodeFactory.instance.objectNode(); + response.put("status", "OK"); + + final var allUpdates = JsonNodeFactory.instance.objectNode(); + updatedConfigs.forEach(configItemChanges -> configItemChanges.addToNode(allUpdates)); + response.set("upgrades", allUpdates); + + channel.sendResponse(new BytesRestResponse(RestStatus.OK, XContentType.JSON.mediaType(), response.toPrettyString())); + }) + .error((status, toXContent) -> response(channel, status, toXContent)); } ValidationResult> applyDifferences( final RestRequest request, final Client client, final List> differencesToUpdate - ) throws IOException { - final var updatedResources = new ArrayList>(); - for (final Tuple difference : differencesToUpdate) { - updatedResources.add( - loadConfiguration(difference.v1(), false, false).map( - configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)).map( - patchResults -> { - final var response = saveAndUpdateConfigs( - securityApiDependencies, - client, - difference.v1(), - patchResults.configuration() - ); - return ValidationResult.success(response.actionGet()); - } - ).map(indexResponse -> { - - final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), difference.v2()); - return ValidationResult.success(itemsGroupedByOperation); - }) - ) + ) { + try { + final var updatedResources = new ArrayList>(); + for (final Tuple difference : differencesToUpdate) { + updatedResources.add( + loadConfiguration(difference.v1(), false, false).map( + configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)).map( + patchResults -> { + final var response = saveAndUpdateConfigs( + securityApiDependencies, + client, + difference.v1(), + patchResults.configuration() + ); + return ValidationResult.success(response.actionGet()); + } + ).map(indexResponse -> { + + final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), difference.v2()); + return ValidationResult.success(itemsGroupedByOperation); + }) + ) + ); + } + + return ValidationResult.merge(updatedResources); + } catch (final Exception ioe) { + LOGGER.debug("Error while applying differences", ioe); + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage("Error applying configuration, see the log file to troubleshoot.") ); } - return ValidationResult.merge(updatedResources); } - private ValidationResult>> verifyHasDifferences(List> diffs) { + ValidationResult>> verifyHasDifferences(List> diffs) { if (diffs.isEmpty()) { return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Unable to upgrade, no differences found")); } @@ -170,34 +183,50 @@ private ValidationResult>> verifyHasDifferences(List return ValidationResult.success(diffs); } - private ValidationResult>> configurationDifferences(final Set configurations) throws IOException { - final var differences = new ArrayList>>(); - for (final var configuration : configurations) { - differences.add(computeDifferenceToUpdate(configuration)); + ValidationResult>> configurationDifferences(final Set configurations) { + try { + final var differences = new ArrayList>>(); + for (final var configuration : configurations) { + differences.add(computeDifferenceToUpdate(configuration)); + } + return ValidationResult.merge(differences); + } catch (final UncheckedIOException ioe) { + LOGGER.error("Error while processing differences", ioe.getCause()); + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage("Error processing configuration, see the log file to troubleshoot.") + ); } - return ValidationResult.merge(differences); } - private ValidationResult> computeDifferenceToUpdate(final CType configType) throws IOException { - return loadConfiguration(configType, false, false).map(activeRoles -> { - final var activeRolesJson = Utils.convertJsonToJackson(activeRoles, false); + ValidationResult> computeDifferenceToUpdate(final CType configType) { + return withIOException(() -> loadConfiguration(configType, false, false).map(activeRoles -> { + final var activeRolesJson = Utils.convertJsonToJackson(activeRoles, true); final var defaultRolesJson = loadConfigFileAsJson(configType); final var rawDiff = JsonDiff.asJson(activeRolesJson, defaultRolesJson, EnumSet.of(DiffFlags.OMIT_VALUE_ON_REMOVE)); return ValidationResult.success(new Tuple<>(configType, filterRemoveOperations(rawDiff))); - }); + })); } - private ValidationResult> getAndValidateConfigurationsToUpgrade(final RestRequest request) { + ValidationResult> getAndValidateConfigurationsToUpgrade(final RestRequest request) { final String[] configs = request.paramAsStringArray(REQUEST_PARAM_CONFIGS_KEY, null); - final var configurations = Optional.ofNullable(configs).map(CType::fromStringValues).orElse(SUPPORTED_CTYPES); + final Set configurations; + try { + configurations = Optional.ofNullable(configs).map(CType::fromStringValues).orElse(SUPPORTED_CTYPES); + } catch (final IllegalArgumentException iae) { + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage("Found invalid configuration option, valid options are: " + CType.lcStringValues()) + ); + } if (!configurations.stream().allMatch(SUPPORTED_CTYPES::contains)) { // Remove all supported configurations configurations.removeAll(SUPPORTED_CTYPES); return ValidationResult.error( RestStatus.BAD_REQUEST, - badRequestMessage("Unsupported configurations for upgrade" + configurations) + badRequestMessage("Unsupported configurations for upgrade, " + configurations) ); } @@ -232,13 +261,17 @@ private static boolean isRemoveOperation(final JsonNode node) { return node.get("op").asText().equals("remove"); } - public JsonNode loadConfigFileAsJson(final CType cType) throws IOException { + SecurityDynamicConfiguration loadYamlFile(final String filepath, final CType cType) throws IOException { + return ConfigHelper.fromYamlFile(filepath, cType, ConfigurationRepository.DEFAULT_CONFIG_VERSION, 0, 0); + } + + JsonNode loadConfigFileAsJson(final CType cType) throws IOException { final var cd = securityApiDependencies.configurationRepository().getConfigDirectory(); final var filepath = cType.configFile(Path.of(cd)).toString(); try { return AccessController.doPrivileged((PrivilegedExceptionAction) () -> { - var loadedConfiguration = ConfigHelper.fromYamlFile(filepath, cType, ConfigurationRepository.DEFAULT_CONFIG_VERSION, 0, 0); - return Utils.convertJsonToJackson(loadedConfiguration, false); + final var loadedConfiguration = loadYamlFile(filepath, cType); + return Utils.convertJsonToJackson(loadedConfiguration, true); }); } catch (final PrivilegedActionException e) { LOGGER.error("Error when loading configuration from file", e); @@ -270,6 +303,18 @@ public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() { return securityApiDependencies.restApiAdminPrivilegesEvaluator(); } + @Override + public ValidationResult entityReserved(SecurityConfiguration securityConfiguration) { + // Allow modification of reserved entities + return ValidationResult.success(securityConfiguration); + } + + @Override + public ValidationResult entityHidden(SecurityConfiguration securityConfiguration) { + // Allow modification of hidden entities + return ValidationResult.success(securityConfiguration); + } + @Override public RequestContentValidator createRequestContentValidator(final Object... params) { return new ConfigUpgradeContentValidator(new RequestContentValidator.ValidationContext() { @@ -301,9 +346,8 @@ protected ConfigUpgradeContentValidator(final ValidationContext validationContex @Override public ValidationResult validate(final RestRequest request, final JsonNode jsonContent) throws IOException { - return validateContentSize(jsonContent);// .map(this::validateJsonKeys); TODO use this on the request but disable on patching + return validateContentSize(jsonContent); } - } /** Tranforms config changes from a raw PATCH into simplier view */ diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java new file mode 100644 index 0000000000..b8332104c3 --- /dev/null +++ b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java @@ -0,0 +1,274 @@ +package org.opensearch.security.dlic.rest.api; + +import java.io.IOException; +import java.util.List; +import java.util.function.Consumer; + +import com.google.common.collect.ImmutableList; +import com.fasterxml.jackson.databind.node.ObjectNode; +import org.junit.Before; +import org.junit.Test; + +import org.opensearch.action.index.IndexResponse; +import org.opensearch.client.Client; +import org.opensearch.common.action.ActionFuture; +import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestRequest; +import org.opensearch.rest.RestResponse; +import org.opensearch.security.DefaultObjectMapper; +import org.opensearch.security.dlic.rest.support.Utils; +import org.opensearch.security.dlic.rest.validation.ValidationResult; +import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; + +import org.mockito.Mock; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.*; + +public class ConfigUpgradeApiActionUnitTest extends AbstractApiActionValidationTest { + + @Mock + private Client client; + + @Mock + private RestChannel restChannel; + + @Mock + private RestRequest restRequest; + + private ConfigUpgradeApiAction configUpgradeApiAction; + + @Before + public void setUp() throws IOException { + setupRolesConfiguration(); + doReturn(XContentFactory.jsonBuilder()).when(restChannel).newBuilder(); + + final var actionFuture = mock(ActionFuture.class); + doReturn(mock(IndexResponse.class)).when(actionFuture).actionGet(); + doReturn(actionFuture).when(client).index(any()); + + configUpgradeApiAction = spy(new ConfigUpgradeApiAction(clusterService, threadPool, securityApiDependencies)); + + final var objectMapper = DefaultObjectMapper.objectMapper; + final var config = objectMapper.createObjectNode(); + config.set("_meta", objectMapper.createObjectNode().put("type", CType.ROLES.toLCString()).put("config_version", 2)); + config.set("kibana_read_only", objectMapper.createObjectNode().put("reserved", true)); + final var newRole = objectMapper.createObjectNode(); + newRole.put("reserved", true); + newRole.putArray("cluster_permissions").add("test-permission-1").add("test-permission-2"); + config.set("new_role", newRole); + + doReturn(config).when(configUpgradeApiAction).loadConfigFileAsJson(any()); + } + + @Test + public void testCanUpgrade_ErrorLoadingConfig() throws Exception { + // Setup + doThrow(new IOException("abc")).when(configUpgradeApiAction).loadConfigFileAsJson(any()); + + // Execute + configUpgradeApiAction.canUpgrade(restChannel, restRequest, client); + + // Assert + verify(restChannel).sendResponse(verifyResponseBody(body -> assertThat(body, containsString("see the log file to troubleshoot")))); + } + + @Test + public void testPerformUpgrade_ErrorLoadingConfig() throws Exception { + // Setup + doThrow(new IOException("abc")).when(configUpgradeApiAction).loadConfigFileAsJson(any()); + + // Execute + configUpgradeApiAction.performUpgrade(restChannel, restRequest, client); + + // Assert + verify(restChannel).sendResponse(verifyResponseBody(body -> assertThat(body, containsString("see the log file to troubleshoot")))); + } + + @Test + public void testPerformUpgrade_ErrorApplyConfig() throws Exception { + // Setup + doThrow(new RuntimeException("abc")).when(configUpgradeApiAction).patchEntities(any(), any(), any()); + + // Execute + configUpgradeApiAction.performUpgrade(restChannel, restRequest, client); + + // Assert + verify(restChannel).sendResponse(verifyResponseBody(body -> assertThat(body, containsString("see the log file to troubleshoot")))); + } + + @Test + public void testPerformUpgrade_NoDifferences() throws Exception { + // Setup + final var rolesCopy = rolesConfiguration.deepClone(); + rolesCopy.removeStatic(); // Statics are added by code, not by config files, they should be omitted + final var rolesJsonNode = Utils.convertJsonToJackson(rolesCopy, true); + doReturn(rolesJsonNode).when(configUpgradeApiAction).loadConfigFileAsJson(any()); + + // Execute + configUpgradeApiAction.performUpgrade(restChannel, restRequest, client); + + // Verify + verify(restChannel).sendResponse(verifyResponseBody(body -> assertThat(body, containsString("no differences found")))); + } + + @Test + public void testPerformUpgrade_WithDifferences() throws Exception { + // Execute + configUpgradeApiAction.performUpgrade(restChannel, restRequest, client); + + // Verify + verify(restChannel).sendResponse(argThat(response -> { + String content = response.content().utf8ToString(); + assertThat(content, equalTo("{\n" + // + " \"status\" : \"OK\",\n" + // + " \"upgrades\" : {\n" + // + " \"roles\" : {\n" + // + " \"add\" : [ \"new_role\" ]\n" + // + " }\n" + // + " }\n" + // + "}")); + return true; + })); + } + + @Test + public void testConfigurationDifferences_OperationBash() throws IOException { + final var testCases = new ImmutableList.Builder(); + + testCases.add( + new OperationTestCase("Missing entry", source -> {}, updated -> updated.put("a", "1"), List.of(List.of("add", "/a", "1"))) + ); + + testCases.add( + new OperationTestCase( + "Same object", + source -> source.set("a", objectMapper.createObjectNode()), + updated -> updated.set("a", objectMapper.createObjectNode()), + List.of() + ) + ); + + testCases.add( + new OperationTestCase("Missing object", source -> source.set("a", objectMapper.createObjectNode()), updated -> {}, List.of()) + ); + + testCases.add(new OperationTestCase("Moved and identical object", source -> { + source.set("a", objectMapper.createObjectNode()); + source.set("b", objectMapper.createObjectNode()); + source.set("c", objectMapper.createObjectNode()); + }, updated -> { + updated.set("a", objectMapper.createObjectNode()); + updated.set("c", objectMapper.createObjectNode()); + updated.set("b", objectMapper.createObjectNode()); + }, List.of())); + + testCases.add(new OperationTestCase("Moved and different object", source -> { + source.set("a", objectMapper.createObjectNode()); + source.set("b", objectMapper.createObjectNode()); + source.set("c", objectMapper.createObjectNode()); + }, updated -> { + updated.set("a", objectMapper.createObjectNode()); + updated.set("c", objectMapper.createObjectNode().put("d", "1")); + updated.set("b", objectMapper.createObjectNode()); + }, List.of(List.of("add", "/c/d", "1")))); + + testCases.add(new OperationTestCase("Removed field object", source -> { + source.set("a", objectMapper.createObjectNode().put("hidden", true)); + source.set("b", objectMapper.createObjectNode()); + }, updated -> { + updated.set("a", objectMapper.createObjectNode()); + updated.set("b", objectMapper.createObjectNode()); + }, List.of(List.of("remove", "/a/hidden", "")))); + + testCases.add(new OperationTestCase("Removed field object", source -> { + final var roleA = objectMapper.createObjectNode(); + roleA.putArray("cluster_permissions").add("1").add("2").add("3"); + source.set("a", roleA); + }, updated -> { + final var roleA = objectMapper.createObjectNode(); + roleA.putArray("cluster_permissions").add("2").add("11").add("3").add("44"); + updated.set("a", roleA); + }, + List.of( + List.of("remove", "/a/cluster_permissions/0", ""), + List.of("add", "/a/cluster_permissions/1", "11"), + List.of("add", "/a/cluster_permissions/3", "44") + ) + )); + + for (final var tc : testCases.build()) { + // Setup + final var source = objectMapper.createObjectNode(); + source.set("_meta", objectMapper.createObjectNode().put("type", CType.ROLES.toLCString()).put("config_version", 2)); + tc.sourceChanges.accept(source); + final var updated = objectMapper.createObjectNode(); + tc.updates.accept(updated); + + var sourceAsConfig = SecurityDynamicConfiguration.fromJson(objectMapper.writeValueAsString(source), CType.ROLES, 2, 1, 1); + + doReturn(ValidationResult.success(sourceAsConfig)).when(configUpgradeApiAction) + .loadConfiguration(any(), anyBoolean(), anyBoolean()); + doReturn(updated).when(configUpgradeApiAction).loadConfigFileAsJson(any()); + + // Execute + var result = configUpgradeApiAction.computeDifferenceToUpdate(CType.ACTIONGROUPS); + + // Verify + result.valid(differences -> { + assertThat(differences.v1(), equalTo(CType.ACTIONGROUPS)); + assertThat(tc.name + ": Number of operations", differences.v2().size(), equalTo(tc.expectedResults.size())); + final var expectedResultsIterator = tc.expectedResults.iterator(); + differences.v2().forEach(operation -> { + final List expected = expectedResultsIterator.next(); + assertThat( + tc.name + ": Operation type" + operation.toPrettyString(), + operation.get("op").asText(), + equalTo(expected.get(0)) + ); + assertThat(tc.name + ": Path" + operation.toPrettyString(), operation.get("path").asText(), equalTo(expected.get(1))); + assertThat( + tc.name + ": Value " + operation.toPrettyString(), + operation.has("value") ? operation.get("value").asText("") : "", + equalTo(expected.get(2)) + ); + }); + }); + } + } + + static class OperationTestCase { + final String name; + final Consumer sourceChanges; + final Consumer updates; + final List> expectedResults; + + OperationTestCase( + final String name, + final Consumer sourceChanges, + final Consumer updates, + final List> expectedResults + ) { + this.name = name; + this.sourceChanges = sourceChanges; + this.updates = updates; + this.expectedResults = expectedResults; + } + + } + + private RestResponse verifyResponseBody(final Consumer test) { + return argThat(response -> { + final String content = response.content().utf8ToString(); + test.accept(content); + return true; + }); + } + +} From ddc04e518229cc060c0b9dda0dda572578a97842 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 8 Mar 2024 21:30:12 +0000 Subject: [PATCH 18/22] Clean up tests Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 69 +++++++++++-------- .../dlic/rest/api/ConfigUpgradeApiAction.java | 11 ++- .../api/ConfigUpgradeApiActionUnitTest.java | 24 +++++-- 3 files changed, 66 insertions(+), 38 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index fe6bf2d7a3..18822d1d5d 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -87,38 +87,53 @@ public void securityRolesUgrade() throws Exception { try (var client = cluster.getRestClient(ADMIN_USER)) { Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); - final var defaultRolesResponse = client.get("_plugins/_security/api/roles/"); - final var rolesNames = extractFieldNames(defaultRolesResponse.getBodyAs(JsonNode.class)); + final var expectedRoles = client.get("_plugins/_security/api/roles/"); + final var expectedRoleNames = extractFieldNames(expectedRoles.getBodyAs(JsonNode.class)); - final var checkForUpgrade = client.get("_plugins/_security/api/_upgrade_check"); - System.out.println("checkForUpgrade Response: " + checkForUpgrade.getBody()); + final var upgradeCheck = client.get("_plugins/_security/api/_upgrade_check"); + upgradeCheck.assertStatusCode(200); + assertThat(upgradeCheck.getBooleanFromJsonBody("/upgradeAvaliable"), equalTo(false)); final var roleToDelete = "flow_framework_full_access"; - final var deleteRoleResponse = client.delete("_plugins/_security/api/roles/" + roleToDelete); - deleteRoleResponse.assertStatusCode(200); - - final var checkForUpgrade3 = client.get("_plugins/_security/api/_upgrade_check"); - System.out.println("checkForUpgrade3 Response: " + checkForUpgrade3.getBody()); + client.delete("_plugins/_security/api/roles/" + roleToDelete).assertStatusCode(200); final var roleToAlter = "flow_framework_read_access"; - final String patchBody = "[{ \"op\": \"replace\", \"path\": \"/cluster_permissions\", \"value\":" - + "[\"a\",\"b\",\"c\"]" - + "},{ \"op\": \"add\", \"path\": \"/index_permissions\", \"value\":" - + "[{\"index_patterns\":[\"*\"],\"allowed_actions\":[\"*\"]}]" - + "}]"; - final var updateRoleResponse = client.patch("_plugins/_security/api/roles/" + roleToAlter, patchBody); - updateRoleResponse.assertStatusCode(200); - System.out.println("Updated Role Response: " + updateRoleResponse.getBody()); - - final var checkForUpgrade2 = client.get("_plugins/_security/api/_upgrade_check"); - System.out.println("checkForUpgrade2 Response: " + checkForUpgrade2.getBody()); - - final var upgradeResponse = client.post("_plugins/_security/api/_upgrade_perform"); - System.out.println("upgrade Response: " + upgradeResponse.getBody()); - - final var afterUpgradeRolesResponse = client.get("_plugins/_security/api/roles/"); - final var afterUpgradeRolesNames = extractFieldNames(afterUpgradeRolesResponse.getBodyAs(JsonNode.class)); - assertThat(afterUpgradeRolesResponse.getBody(), afterUpgradeRolesNames, equalTo(rolesNames)); + client.patch("_plugins/_security/api/roles/" + roleToAlter, "[\n" + // + " {\n" + // + " \"op\": \"replace\",\n" + // + " \"path\": \"/cluster_permissions\",\n" + // + " \"value\": [\"a\", \"b\", \"c\"]\n" + // + " },\n" + // + " {\n" + // + " \"op\": \"add\",\n" + // + " \"path\": \"/index_permissions\",\n" + // + " \"value\": [ {\n" + // + " \"index_patterns\": [\"*\"],\n" + // + " \"allowed_actions\": [\"*\"]\n" + // + " }\n" + // + " ]\n" + // + " }\n" + // + "]").assertStatusCode(200); + + final var upgradeCheckAfterChanges = client.get("_plugins/_security/api/_upgrade_check"); + upgradeCheckAfterChanges.assertStatusCode(200); + assertThat( + upgradeCheckAfterChanges.getTextArrayFromJsonBody("/upgradeActions/roles/add"), + equalTo(List.of("flow_framework_full_access")) + ); + assertThat( + upgradeCheckAfterChanges.getTextArrayFromJsonBody("/upgradeActions/roles/modify"), + equalTo(List.of("flow_framework_read_access")) + ); + + final var performUpgrade = client.post("_plugins/_security/api/_upgrade_perform"); + performUpgrade.assertStatusCode(200); + assertThat(performUpgrade.getTextArrayFromJsonBody("/upgrades/roles/add"), equalTo(List.of("flow_framework_full_access"))); + assertThat(performUpgrade.getTextArrayFromJsonBody("/upgrades/roles/modify"), equalTo(List.of("flow_framework_read_access"))); + + final var afterUpgradeRoles = client.get("_plugins/_security/api/roles/"); + final var afterUpgradeRolesNames = extractFieldNames(afterUpgradeRoles.getBodyAs(JsonNode.class)); + assertThat(afterUpgradeRolesNames, equalTo(expectedRoleNames)); } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 0cb51e3bb5..063f0f7281 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -47,7 +47,6 @@ import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; import org.opensearch.security.configuration.ConfigurationRepository; -import org.opensearch.security.dlic.rest.api.ConfigUpgradeApiAction.ConfigItemChanges; import org.opensearch.security.dlic.rest.support.Utils; import org.opensearch.security.dlic.rest.validation.EndpointValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; @@ -127,7 +126,7 @@ void performUpgrade(final RestChannel channel, final RestRequest request, final .error((status, toXContent) -> response(channel, status, toXContent)); } - ValidationResult> applyDifferences( + private ValidationResult> applyDifferences( final RestRequest request, final Client client, final List> differencesToUpdate @@ -183,7 +182,7 @@ ValidationResult>> verifyHasDifferences(List>> configurationDifferences(final Set configurations) { + private ValidationResult>> configurationDifferences(final Set configurations) { try { final var differences = new ArrayList>>(); for (final var configuration : configurations) { @@ -208,7 +207,7 @@ ValidationResult> computeDifferenceToUpdate(final CType c })); } - ValidationResult> getAndValidateConfigurationsToUpgrade(final RestRequest request) { + private ValidationResult> getAndValidateConfigurationsToUpgrade(final RestRequest request) { final String[] configs = request.paramAsStringArray(REQUEST_PARAM_CONFIGS_KEY, null); final Set configurations; @@ -233,7 +232,7 @@ ValidationResult> getAndValidateConfigurationsToUpgrade(final RestReq return ValidationResult.success(configurations); } - JsonNode filterRemoveOperations(final JsonNode diff) { + private JsonNode filterRemoveOperations(final JsonNode diff) { final ArrayNode filteredDiff = JsonNodeFactory.instance.arrayNode(); diff.forEach(node -> { if (!isRemoveOperation(node)) { @@ -261,7 +260,7 @@ private static boolean isRemoveOperation(final JsonNode node) { return node.get("op").asText().equals("remove"); } - SecurityDynamicConfiguration loadYamlFile(final String filepath, final CType cType) throws IOException { + private SecurityDynamicConfiguration loadYamlFile(final String filepath, final CType cType) throws IOException { return ConfigHelper.fromYamlFile(filepath, cType, ConfigurationRepository.DEFAULT_CONFIG_VERSION, 0, 0); } diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java index b8332104c3..8f9ef2b2c7 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java @@ -1,3 +1,14 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + package org.opensearch.security.dlic.rest.api; import java.io.IOException; @@ -8,7 +19,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode; import org.junit.Before; import org.junit.Test; - +import org.mockito.Mock; import org.opensearch.action.index.IndexResponse; import org.opensearch.client.Client; import org.opensearch.common.action.ActionFuture; @@ -22,14 +33,17 @@ import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; -import org.mockito.Mock; - import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; public class ConfigUpgradeApiActionUnitTest extends AbstractApiActionValidationTest { From c906ac1feac813aee97d9fbf191fd5383f5a3de5 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 8 Mar 2024 21:38:30 +0000 Subject: [PATCH 19/22] Spotless fix Signed-off-by: Peter Nied --- .../dlic/rest/api/ConfigUpgradeApiActionUnitTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java index 8f9ef2b2c7..f39f9185fa 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java @@ -19,7 +19,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode; import org.junit.Before; import org.junit.Test; -import org.mockito.Mock; + import org.opensearch.action.index.IndexResponse; import org.opensearch.client.Client; import org.opensearch.common.action.ActionFuture; @@ -33,6 +33,8 @@ import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; +import org.mockito.Mock; + import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; From 89fa8a9486d66c76d26ce7f3718d0cdcddbfc1e3 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 8 Mar 2024 22:46:41 +0000 Subject: [PATCH 20/22] Fix minor typo and look into test failures Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 2 +- .../dlic/rest/api/ConfigUpgradeApiAction.java | 7 ++++--- .../api/AbstractApiActionValidationTest.java | 4 +++- .../api/ConfigUpgradeApiActionUnitTest.java | 5 +++-- .../InternalUsersApiActionValidationTest.java | 20 +++++++++++++++---- 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index 18822d1d5d..c33c99dfdd 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -92,7 +92,7 @@ public void securityRolesUgrade() throws Exception { final var upgradeCheck = client.get("_plugins/_security/api/_upgrade_check"); upgradeCheck.assertStatusCode(200); - assertThat(upgradeCheck.getBooleanFromJsonBody("/upgradeAvaliable"), equalTo(false)); + assertThat(upgradeCheck.getBooleanFromJsonBody("/upgradeAvailable"), equalTo(false)); final var roleToDelete = "flow_framework_full_access"; client.delete("_plugins/_security/api/roles/" + roleToDelete).assertStatusCode(200); diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 063f0f7281..2d04b72892 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -95,12 +95,13 @@ void canUpgrade(final RestChannel channel, final RestRequest request, final Clie .map(kvp -> new ConfigItemChanges(kvp.v1(), kvp.v2())) .collect(Collectors.toList()); - final var upgradeAvaliable = allConfigItemChanges.stream().anyMatch(ConfigItemChanges::hasChanges); + final var upgradeAvailable = allConfigItemChanges.stream().anyMatch(ConfigItemChanges::hasChanges); final ObjectNode response = JsonNodeFactory.instance.objectNode(); - response.put("upgradeAvaliable", upgradeAvaliable); + response.put("status", "OK"); + response.put("upgradeAvailable", upgradeAvailable); - if (upgradeAvaliable) { + if (upgradeAvailable) { final ObjectNode differences = JsonNodeFactory.instance.objectNode(); allConfigItemChanges.forEach(configItemChanges -> configItemChanges.addToNode(differences)); response.set("upgradeActions", differences); diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java index f2df09549f..4b2e9e4417 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java @@ -116,10 +116,12 @@ protected CType getConfigType() { } - protected JsonNode xContentToJsonNode(final ToXContent toXContent) throws IOException { + protected JsonNode xContentToJsonNode(final ToXContent toXContent) { try (final var xContentBuilder = XContentFactory.jsonBuilder()) { toXContent.toXContent(xContentBuilder, ToXContent.EMPTY_PARAMS); return DefaultObjectMapper.readTree(xContentBuilder.toString()); + } catch (final IOException ioe) { + throw new RuntimeException(ioe); } } diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java index f39f9185fa..36407cfbc4 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java @@ -141,8 +141,9 @@ public void testPerformUpgrade_WithDifferences() throws Exception { // Verify verify(restChannel).sendResponse(argThat(response -> { - String content = response.content().utf8ToString(); - assertThat(content, equalTo("{\n" + // + final var rawResponseBody = response.content().utf8ToString(); + final var newlineNormalizedBody = rawResponseBody.replace("\r\n", "\n"); + assertThat(newlineNormalizedBody, equalTo("{\n" + // " \"status\" : \"OK\",\n" + // " \"upgrades\" : {\n" + // " \"roles\" : {\n" + // diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java index 773d356246..2af598f5d5 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java @@ -22,6 +22,7 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.rest.RestRequest; import org.opensearch.security.DefaultObjectMapper; +import org.opensearch.security.dlic.rest.validation.ValidationResult; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; import org.opensearch.security.securityconf.impl.v7.InternalUserV7; @@ -32,9 +33,13 @@ import org.mockito.Mock; import org.mockito.Mockito; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.Mockito.when; public class InternalUsersApiActionValidationTest extends AbstractApiActionValidationTest { @@ -145,19 +150,26 @@ public void validateSecurityRolesWithMutableRolesMappingConfig() throws Exceptio // should ok to set regular role with mutable role mapping var userJson = objectMapper.createObjectNode().set("opendistro_security_roles", objectMapper.createArrayNode().add("regular_role")); var result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration)); - assertTrue(result.isValid()); + assertValidationResultIsValid(result); // should be ok to set reserved role with mutable role mapping userJson = objectMapper.createObjectNode().set("opendistro_security_roles", objectMapper.createArrayNode().add("kibana_read_only")); result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration)); - assertTrue(result.isValid()); + assertValidationResultIsValid(result); // should be ok to set static role with mutable role mapping userJson = objectMapper.createObjectNode().set("opendistro_security_roles", objectMapper.createArrayNode().add("all_access")); result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration)); - assertTrue(result.isValid()); + assertValidationResultIsValid(result); // should not be ok to set hidden role with mutable role mapping userJson = objectMapper.createObjectNode().set("opendistro_security_roles", objectMapper.createArrayNode().add("some_hidden_role")); result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration)); - assertFalse(result.isValid()); + final var errorMessage = xContentToJsonNode(result.errorMessage()).toPrettyString(); + assertThat(errorMessage, allOf(containsString("NOT_FOUND"), containsString("Resource 'some_hidden_role' is not available."))); + } + + void assertValidationResultIsValid(final ValidationResult result) { + if (!result.isValid()) { + fail("Expected valid result, error message: " + xContentToJsonNode(result.errorMessage()).toPrettyString()); + } } @Test From c40f46f2702fef154d8be93a28fd5c377c42f0fb Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 15 Mar 2024 20:36:14 +0000 Subject: [PATCH 21/22] Add comments and validation around the upgrade integration test Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index c33c99dfdd..eb028c74e4 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -36,6 +36,7 @@ import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasKey; +import static org.hamcrest.Matchers.not; @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) @@ -85,20 +86,26 @@ public void shouldLoadDefaultConfiguration() { @Test public void securityRolesUgrade() throws Exception { try (var client = cluster.getRestClient(ADMIN_USER)) { + // Setup: Make sure the config is ready before starting modifications Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); + // Setup: Collect default roles after cluster start final var expectedRoles = client.get("_plugins/_security/api/roles/"); final var expectedRoleNames = extractFieldNames(expectedRoles.getBodyAs(JsonNode.class)); + // Verify: Before any changes, nothing to upgrade final var upgradeCheck = client.get("_plugins/_security/api/_upgrade_check"); upgradeCheck.assertStatusCode(200); assertThat(upgradeCheck.getBooleanFromJsonBody("/upgradeAvailable"), equalTo(false)); + // Action: Select a role that is part of the defaults and delete that role final var roleToDelete = "flow_framework_full_access"; client.delete("_plugins/_security/api/roles/" + roleToDelete).assertStatusCode(200); + // Action: Select a role that is part of the defaults and alter that role with removal, edits, and additions final var roleToAlter = "flow_framework_read_access"; - client.patch("_plugins/_security/api/roles/" + roleToAlter, "[\n" + // + final var originalRoleConfig = client.get("_plugins/_security/api/roles/" + roleToAlter).getBodyAs(JsonNode.class); + final var alteredRoleReponse = client.patch("_plugins/_security/api/roles/" + roleToAlter, "[\n" + // " {\n" + // " \"op\": \"replace\",\n" + // " \"path\": \"/cluster_permissions\",\n" + // @@ -113,8 +120,12 @@ public void securityRolesUgrade() throws Exception { " }\n" + // " ]\n" + // " }\n" + // - "]").assertStatusCode(200); + "]"); + alteredRoleReponse.assertStatusCode(200); + final var alteredRoleJson = alteredRoleReponse.getBodyAs(JsonNode.class); + assertThat(originalRoleConfig, not(equalTo(alteredRoleJson))); + // Verify: Confirm that the upgrade check detects the changes associated with both role resources final var upgradeCheckAfterChanges = client.get("_plugins/_security/api/_upgrade_check"); upgradeCheckAfterChanges.assertStatusCode(200); assertThat( @@ -126,14 +137,20 @@ public void securityRolesUgrade() throws Exception { equalTo(List.of("flow_framework_read_access")) ); + // Action: Perform the upgrade to the roles configuration final var performUpgrade = client.post("_plugins/_security/api/_upgrade_perform"); performUpgrade.assertStatusCode(200); assertThat(performUpgrade.getTextArrayFromJsonBody("/upgrades/roles/add"), equalTo(List.of("flow_framework_full_access"))); assertThat(performUpgrade.getTextArrayFromJsonBody("/upgrades/roles/modify"), equalTo(List.of("flow_framework_read_access"))); + // Verify: Same roles as the original state - the deleted role has been restored final var afterUpgradeRoles = client.get("_plugins/_security/api/roles/"); final var afterUpgradeRolesNames = extractFieldNames(afterUpgradeRoles.getBodyAs(JsonNode.class)); assertThat(afterUpgradeRolesNames, equalTo(expectedRoleNames)); + + // Verify: Altered role was restored to its expected state + final var afterUpgradeAlteredRoleConfig = client.get("_plugins/_security/api/roles/" + roleToAlter).getBodyAs(JsonNode.class); + assertThat(originalRoleConfig, equalTo(afterUpgradeAlteredRoleConfig)); } } From b71ee7552276bf149989c9481bb96b61114c4062 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Mon, 18 Mar 2024 21:29:14 +0000 Subject: [PATCH 22/22] Add comments around ConfigItemChanges methods Signed-off-by: Peter Nied --- .../security/dlic/rest/api/ConfigUpgradeApiAction.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 2d04b72892..f295ab8c1c 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -365,6 +365,7 @@ public boolean hasChanges() { return !itemsGroupedByOperation.isEmpty(); } + /** Adds the config item changes to the json node */ public void addToNode(final ObjectNode node) { final var allOperations = JsonNodeFactory.instance.objectNode(); itemsGroupedByOperation.forEach((operation, items) -> { @@ -374,6 +375,10 @@ public void addToNode(final ObjectNode node) { node.set(config.toLCString(), allOperations); } + /** + * Classifies the changes to this config into groupings by the type of change, for + * multiple changes types on the same item they are groupped as 'modify' + */ private static Map> classifyChanges(final JsonNode differences) { final var items = new HashMap(); differences.forEach(node -> {