From 35481aaf2bc5b62f6892d28f76177074bc782e67 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Wed, 25 Sep 2024 08:01:02 +0200 Subject: [PATCH] Fixes Signed-off-by: Nils Bandener --- .../LegacyConfigV6AutoConversionTest.java | 96 +++++++++++-------- .../ConfigurationLoaderSecurity7.java | 5 +- .../dlic/rest/api/AbstractApiAction.java | 1 - .../security/securityconf/impl/CType.java | 2 +- .../impl/SecurityDynamicConfiguration.java | 43 +++++++-- .../api/AbstractApiActionValidationTest.java | 7 +- .../InternalUsersApiActionValidationTest.java | 6 +- .../RolesMappingApiActionValidationTest.java | 6 -- 8 files changed, 95 insertions(+), 71 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java b/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java index 4311cd268c..b31cdfaf48 100644 --- a/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java +++ b/src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java @@ -9,66 +9,70 @@ */ package org.opensearch.security.legacy; +import java.util.Map; + import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; import org.junit.Assert; import org.junit.ClassRule; import org.junit.FixMethodOrder; import org.junit.Test; import org.junit.runner.RunWith; - import org.junit.runners.MethodSorters; + import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; -import java.util.Map; - -@FixMethodOrder( MethodSorters.NAME_ASCENDING ) +@FixMethodOrder(MethodSorters.NAME_ASCENDING) @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) public class LegacyConfigV6AutoConversionTest { static final TestSecurityConfig LEGACY_CONFIG = new TestSecurityConfig()// - .rawConfigurationDocumentYaml("config", "opendistro_security:\n" + - " dynamic:\n" + - " authc:\n" + - " basic_internal_auth_domain:\n" + - " http_enabled: true\n" + - " order: 4\n" + - " http_authenticator:\n" + - " type: basic\n" + - " challenge: true\n" + - " authentication_backend:\n" + - " type: intern\n")// - .rawConfigurationDocumentYaml("internalusers", "admin:\n" + - " readonly: true\n" + - " hash: $2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG\n" + - " roles:\n" + - " - admin\n" + - " attributes:\n" + - " attribute1: value1\n")// - .rawConfigurationDocumentYaml("roles", "all_access_role:\n" + - " readonly: true\n" + - " cluster:\n" + - " - UNLIMITED\n" + - " indices:\n" + - " '*':\n" + - " '*':\n" + - " - UNLIMITED\n" + - " tenants:\n" + - " admin_tenant: RW\n")// - .rawConfigurationDocumentYaml("rolesmapping", "all_access_role:\n" + - " readonly: true\n" + - " backendroles:\n" + - " - admin")// - .rawConfigurationDocumentYaml("actiongroups", "dummy:\n" + - " permissions: []") - ; + .rawConfigurationDocumentYaml( + "config", + "opendistro_security:\n" + + " dynamic:\n" + + " authc:\n" + + " basic_internal_auth_domain:\n" + + " http_enabled: true\n" + + " order: 4\n" + + " http_authenticator:\n" + + " type: basic\n" + + " challenge: true\n" + + " authentication_backend:\n" + + " type: intern\n" + )// + .rawConfigurationDocumentYaml( + "internalusers", + "admin:\n" + + " readonly: true\n" + + " hash: $2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG\n" + + " roles:\n" + + " - admin\n" + + " attributes:\n" + + " attribute1: value1\n" + )// + .rawConfigurationDocumentYaml( + "roles", + "all_access_role:\n" + + " readonly: true\n" + + " cluster:\n" + + " - UNLIMITED\n" + + " indices:\n" + + " '*':\n" + + " '*':\n" + + " - UNLIMITED\n" + + " tenants:\n" + + " admin_tenant: RW\n" + )// + .rawConfigurationDocumentYaml("rolesmapping", "all_access_role:\n" + " readonly: true\n" + " backendroles:\n" + " - admin")// + .rawConfigurationDocumentYaml("actiongroups", "dummy:\n" + " permissions: []"); @ClassRule public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) .config(LEGACY_CONFIG) - .nodeSettings(Map.of("plugins.security.restapi.roles_enabled.0", "all_access_role")) + .nodeSettings(Map.of("plugins.security.restapi.roles_enabled.0", "all_access_role")) .build(); @Test @@ -85,7 +89,11 @@ public void configRestApiReturnsV6Config() { try (TestRestClient client = cluster.getRestClient("admin", "admin")) { TestRestClient.HttpResponse response = client.get("_opendistro/_security/api/roles/all_access_role"); Assert.assertEquals(response.getBody(), 200, response.getStatusCode()); - Assert.assertEquals("Expected v6 format", "{\"all_access_role\":{\"readonly\":true,\"hidden\":false,\"cluster\":[\"UNLIMITED\"],\"tenants\":{\"admin_tenant\":\"RW\"},\"indices\":{\"*\":{\"*\":[\"UNLIMITED\"]}}}}", response.getBody()); + Assert.assertEquals( + "Expected v6 format", + "{\"all_access_role\":{\"readonly\":true,\"hidden\":false,\"cluster\":[\"UNLIMITED\"],\"tenants\":{\"admin_tenant\":\"RW\"},\"indices\":{\"*\":{\"*\":[\"UNLIMITED\"]}}}}", + response.getBody() + ); } } @@ -100,7 +108,11 @@ public void zzz_migrateApi() { Assert.assertEquals(response.getBody(), "Migration completed.", response.getTextFromJsonBody("/message")); response = client.get("_opendistro/_security/api/roles/all_access_role"); Assert.assertEquals(response.getBody(), 200, response.getStatusCode()); - Assert.assertEquals("Expected v7 format", "Migrated from v6 (all types mapped)", response.getTextFromJsonBody("/all_access_role/description")); + Assert.assertEquals( + "Expected v7 format", + "Migrated from v6 (all types mapped)", + response.getTextFromJsonBody("/all_access_role/description") + ); } } diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java b/src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java index da193f1212..571bb802db 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java @@ -38,7 +38,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.opensearch.LegacyESVersion; import org.opensearch.action.get.GetResponse; import org.opensearch.action.get.MultiGetItemResponse; import org.opensearch.action.get.MultiGetRequest; @@ -211,7 +210,9 @@ public void failure(Throwable t) { SecurityDynamicConfiguration roleConfig = result.get(CType.ROLES); SecurityDynamicConfiguration tenantConfig = result.get(CType.TENANTS); - if (roleConfig != null && roleConfig.getAutoConvertedFrom() != null && (tenantConfig == null || tenantConfig.getCEntries().isEmpty())) { + if (roleConfig != null + && roleConfig.getAutoConvertedFrom() != null + && (tenantConfig == null || tenantConfig.getCEntries().isEmpty())) { // Special case for configuration that was auto-converted from v6: // We need to generate tenant config from role config. // Having such a special case here is not optimal, but IMHO acceptable, as this 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 903c601143..d1289471a6 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 @@ -12,7 +12,6 @@ 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.Optional; 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 b987e265d1..662174de44 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/CType.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/CType.java @@ -297,7 +297,7 @@ public SecurityDynamicConfiguration parseJson(CType ctype, Str public SecurityDynamicConfiguration convert(SecurityDynamicConfiguration oldConfig, CType ctype) { SecurityDynamicConfiguration newConfig = SecurityDynamicConfiguration.empty(ctype); newConfig.setAutoConvertedFrom(oldConfig); -// newConfig.setSeqNoPrimaryTerm(); + // newConfig.setSeqNoPrimaryTerm(); for (Map.Entry oldEntry : oldConfig.getCEntries().entrySet()) { newConfig.putCEntry(mapKeyConversionFunction.apply(oldEntry.getKey()), entryConversionFunction.apply(oldEntry.getValue())); diff --git a/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java b/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java index c925bdac7a..d99c9ad2d0 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java @@ -86,7 +86,6 @@ public static SecurityDynamicConfiguration empty(CType ctype, int vers return result; } - @JsonIgnore public boolean notEmpty() { return !centries.isEmpty(); @@ -157,11 +156,11 @@ public static SecurityDynamicConfiguration fromJson( * is not the current one, no conversion will be performed. The configuration will be returned as it was found. */ public static SecurityDynamicConfiguration fromJsonWithoutAutoConversion( - String json, - CType ctype, - int version, - long seqNo, - long primaryTerm + String json, + CType ctype, + int version, + long seqNo, + long primaryTerm ) throws IOException { return fromNodeWithoutAutoConversion(DefaultObjectMapper.readTree(json), ctype, version, seqNo, primaryTerm); } @@ -421,12 +420,24 @@ public Class getImplementingClass() { public SecurityDynamicConfiguration deepClone() { try { if (ctype != null) { - SecurityDynamicConfiguration result = fromJson(DefaultObjectMapper.writeValueAsString(this, false), ctype, version, seqNo, primaryTerm); + SecurityDynamicConfiguration result = fromJson( + DefaultObjectMapper.writeValueAsString(this, false), + ctype, + version, + seqNo, + primaryTerm + ); result.autoConvertedFrom = this.autoConvertedFrom; return result; } else { // We are on a pre-v7 config version. This can be only if we skipped auto conversion. So, we do here the same. - SecurityDynamicConfiguration result = (SecurityDynamicConfiguration) fromJsonWithoutAutoConversion(DefaultObjectMapper.writeValueAsString(this, false), ctypeUnsafe, version, seqNo, primaryTerm); + SecurityDynamicConfiguration result = (SecurityDynamicConfiguration) fromJsonWithoutAutoConversion( + DefaultObjectMapper.writeValueAsString(this, false), + ctypeUnsafe, + version, + seqNo, + primaryTerm + ); return result; } } catch (Exception e) { @@ -439,12 +450,24 @@ public SecurityDynamicConfiguration deepClone() { public SecurityDynamicConfiguration deepCloneWithRedaction() { try { if (ctype != null) { - SecurityDynamicConfiguration result = fromJson(DefaultObjectMapper.writeValueAsStringAndRedactSensitive(this), ctype, version, seqNo, primaryTerm); + SecurityDynamicConfiguration result = fromJson( + DefaultObjectMapper.writeValueAsStringAndRedactSensitive(this), + ctype, + version, + seqNo, + primaryTerm + ); result.autoConvertedFrom = this.autoConvertedFrom; return result; } else { // We are on a pre-v7 config version. This can be only if we skipped auto conversion. So, we do here the same. - SecurityDynamicConfiguration result = (SecurityDynamicConfiguration) fromJsonWithoutAutoConversion(DefaultObjectMapper.writeValueAsStringAndRedactSensitive(this), ctypeUnsafe, version, seqNo, primaryTerm); + SecurityDynamicConfiguration result = (SecurityDynamicConfiguration) fromJsonWithoutAutoConversion( + DefaultObjectMapper.writeValueAsStringAndRedactSensitive(this), + ctypeUnsafe, + version, + seqNo, + primaryTerm + ); return result; } } catch (Exception e) { 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 fa51461ac4..ee18a57274 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 @@ -26,7 +26,6 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.security.DefaultObjectMapper; -import org.opensearch.security.configuration.ConfigurationMap; import org.opensearch.security.configuration.ConfigurationRepository; import org.opensearch.security.hasher.PasswordHasher; import org.opensearch.security.hasher.PasswordHasherFactory; @@ -41,7 +40,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.doReturn; @RunWith(MockitoJUnitRunner.class) public abstract class AbstractApiActionValidationTest { @@ -101,9 +100,7 @@ void setupRolesConfiguration() throws IOException { config.set("regular_role", objectMapper.createObjectNode().set("cluster_permissions", objectMapper.createArrayNode().add("*"))); rolesConfiguration = SecurityDynamicConfiguration.fromJson(objectMapper.writeValueAsString(config), CType.ROLES, 2, 1, 1); - when(configurationRepository.getConfigurationsFromIndex(List.of(CType.ROLES), false)).thenReturn( - ConfigurationMap.of(rolesConfiguration) - ); + doReturn(rolesConfiguration).when(configurationRepository).getUnconvertedConfigurationFromIndex(CType.ROLES, false); } @Test 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 ffe3461951..9127818d28 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,7 +22,6 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.rest.RestRequest; import org.opensearch.security.DefaultObjectMapper; -import org.opensearch.security.configuration.ConfigurationMap; import org.opensearch.security.dlic.rest.validation.ValidationResult; import org.opensearch.security.hasher.PasswordHasherFactory; import org.opensearch.security.securityconf.impl.CType; @@ -43,6 +42,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.when; public class InternalUsersApiActionValidationTest extends AbstractApiActionValidationTest { @@ -81,9 +81,7 @@ public void setupRolesAndMappings() throws IOException { 1, 1 ); - when(configurationRepository.getConfigurationsFromIndex(List.of(CType.ROLESMAPPING), false)).thenReturn( - ConfigurationMap.of(rolesMappingConfiguration) - ); + doReturn(rolesMappingConfiguration).when(configurationRepository).getUnconvertedConfigurationFromIndex(CType.ROLESMAPPING, false); } @Test diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java index 21dd372265..7232f99b7e 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java @@ -11,14 +11,10 @@ package org.opensearch.security.dlic.rest.api; -import java.util.List; - import org.junit.Before; import org.junit.Test; import org.opensearch.core.rest.RestStatus; -import org.opensearch.security.configuration.ConfigurationMap; -import org.opensearch.security.securityconf.impl.CType; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -61,8 +57,6 @@ public void isNotAllowedNoRightsToChangeRoleEntity() throws Exception { @Test public void onConfigChangeShouldCheckRoles() throws Exception { when(restApiAdminPrivilegesEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenCallRealMethod(); - when(configurationRepository.getConfigurationsFromIndex(List.of(CType.ROLES), false)) - .thenReturn(ConfigurationMap.of(rolesConfiguration)); final var rolesApiActionEndpointValidator = new RolesMappingApiAction(clusterService, threadPool, securityApiDependencies).createEndpointValidator();