diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index 8bb5b96145..eb028c74e4 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -11,10 +11,13 @@ import java.io.IOException; import java.nio.file.Path; +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; import org.apache.commons.io.FileUtils; import org.awaitility.Awaitility; import org.junit.AfterClass; @@ -22,6 +25,7 @@ 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; @@ -32,16 +36,16 @@ 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) 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) @@ -64,15 +68,95 @@ public static void cleanConfigurationDirectory() throws IOException { @Test public void shouldLoadDefaultConfiguration() { - try (TestRestClient client = cluster.getRestClient(NEW_USER, DEFAULT_PASSWORD)) { + 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_NAME, DEFAULT_PASSWORD)) { - client.confirmCorrectCredentials(ADMIN_USER_NAME); + 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_NAME), hasKey(NEW_USER), hasKey(LIMITED_USER))); + assertThat( + users, + allOf(aMapWithSize(3), hasKey(ADMIN_USER.getName()), hasKey(NEW_USER.getName()), hasKey(LIMITED_USER.getName())) + ); } } + + @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"; + 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" + // + " \"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" + // + "]"); + 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( + 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")) + ); + + // 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)); + } + } + + private Set extractFieldNames(final JsonNode json) { + final var set = new HashSet(); + json.fieldNames().forEachRemaining(set::add); + return set; + } } 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 41a161590a..e5d068d405 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -121,6 +121,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).configFile().toAbsolutePath().toString() + "/opensearch-security/"; + return cd; + } + private void initalizeClusterConfiguration(final boolean installDefaultConfig) { try { LOGGER.info("Background init thread started. Install default config?: " + installDefaultConfig); @@ -133,10 +141,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).configFile().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/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java index 04a1dee74c..157d44da73 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; @@ -226,7 +229,7 @@ protected final ValidationResult patchEntity( ); } - protected final ValidationResult patchEntities( + protected ValidationResult patchEntities( final RestRequest request, final JsonNode patchContent, final SecurityConfiguration securityConfiguration @@ -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) { @@ -367,7 +370,7 @@ protected final ValidationResult loadConfiguration(final ); } - protected final ValidationResult> loadConfiguration( + protected ValidationResult> loadConfiguration( final CType cType, boolean omitSensitiveData, final boolean logComplianceEvent @@ -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 new file mode 100644 index 0000000000..f295ab8c1c --- /dev/null +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -0,0 +1,400 @@ +/* + * 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.io.UncheckedIOException; +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.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +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; +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.securityconf.impl.SecurityDynamicConfiguration; +import org.opensearch.security.support.ConfigHelper; +import org.opensearch.threadpool.ThreadPool; + +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; + +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 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")) + ); + + @Inject + public ConfigUpgradeApiAction( + final ClusterService clusterService, + final ThreadPool threadPool, + final SecurityApiDependencies securityApiDependencies + ) { + super(Endpoint.CONFIG, clusterService, threadPool, securityApiDependencies); + this.requestHandlersBuilder.configureRequestHandlers(rhb -> { + rhb.allMethodsNotImplemented().add(Method.GET, this::canUpgrade).add(Method.POST, this::performUpgrade); + }); + } + + 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()); + + final var upgradeAvailable = allConfigItemChanges.stream().anyMatch(ConfigItemChanges::hasChanges); + + final ObjectNode response = JsonNodeFactory.instance.objectNode(); + response.put("status", "OK"); + response.put("upgradeAvailable", upgradeAvailable); + + if (upgradeAvailable) { + final ObjectNode differences = JsonNodeFactory.instance.objectNode(); + 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)); + } + + 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)); + } + + private ValidationResult> applyDifferences( + final RestRequest request, + final Client client, + final List> differencesToUpdate + ) { + 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.") + ); + } + + } + + 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) { + 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.") + ); + } + } + + 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) { + final String[] configs = request.paramAsStringArray(REQUEST_PARAM_CONFIGS_KEY, null); + + 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) + ); + } + + return ValidationResult.success(configurations); + } + + 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 static String pathRoot(final JsonNode node) { + return node.get("path").asText().split("/")[1]; + } + + private static boolean hasRootLevelPath(final JsonNode node) { + final var jsonPath = node.get("path").asText(); + return jsonPath.charAt(0) == '/' && !jsonPath.substring(1).contains("/"); + } + + private static boolean isRemoveOperation(final JsonNode node) { + return node.get("op").asText().equals("remove"); + } + + private 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) () -> { + final var loadedConfiguration = loadYamlFile(filepath, cType); + return Utils.convertJsonToJackson(loadedConfiguration, true); + }); + } catch (final PrivilegedActionException e) { + LOGGER.error("Error when loading configuration from file", e); + throw (IOException) e.getCause(); + } + } + + @Override + public List routes() { + return routes; + } + + @Override + protected CType getConfigType() { + throw new UnsupportedOperationException("This class supports multiple configuration types"); + } + + @Override + protected EndpointValidator createEndpointValidator() { + return new EndpointValidator() { + + @Override + public Endpoint endpoint() { + return endpoint; + } + + @Override + 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() { + @Override + public Object[] params() { + return params; + } + + @Override + public Settings settings() { + return securityApiDependencies.settings(); + } + + @Override + public Map allowedKeys() { + return Map.of(REQUEST_PARAM_CONFIGS_KEY, DataType.ARRAY); + } + }); + } + }; + } + + /** More permissions validation that default ContentValidator */ + static class ConfigUpgradeContentValidator extends RequestContentValidator { + + protected ConfigUpgradeContentValidator(final ValidationContext validationContext) { + super(validationContext); + } + + @Override + public ValidationResult validate(final RestRequest request, final JsonNode jsonContent) throws IOException { + return validateContentSize(jsonContent); + } + } + + /** 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(); + } + + /** 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) -> { + final var arrayNode = allOperations.putArray(operation); + items.forEach(arrayNode::add); + }); + 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 -> { + 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; + } + } +} 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 9cd551c18f..687705f6c7 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/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 ... 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..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 @@ -12,7 +12,9 @@ package org.opensearch.security.dlic.rest.validation; import java.io.IOException; +import java.util.List; import java.util.Objects; +import java.util.stream.Collectors; import org.opensearch.common.CheckedBiConsumer; import org.opensearch.common.CheckedConsumer; @@ -50,6 +52,22 @@ public static ValidationResult error(final RestStatus status, final ToXCo return new ValidationResult<>(status, errorMessage); } + /** + * 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 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 { if (content != null) { return Objects.requireNonNull(mapper).apply(content); @@ -82,5 +100,4 @@ public boolean isValid() { public ToXContent errorMessage() { return errorMessage; } - } 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(); } } 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 new file mode 100644 index 0000000000..36407cfbc4 --- /dev/null +++ b/src/test/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiActionUnitTest.java @@ -0,0 +1,291 @@ +/* + * 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.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.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.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 { + + @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 -> { + 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" + // + " \"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; + }); + } + +} 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