From 89fa8a9486d66c76d26ce7f3718d0cdcddbfc1e3 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 8 Mar 2024 22:46:41 +0000 Subject: [PATCH] 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