Fixes

Signed-off-by: Nils Bandener <[email protected]>
opensearch-project · Sep 25, 2024 · 35481aa · 35481aa
1 parent 50bc999
commit 35481aa
Show file tree

Hide file tree

Showing 8 changed files with 95 additions and 71 deletions.
diff --git a/...integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java b/...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
@@ -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<RoleV7> roleConfig = result.get(CType.ROLES);
         SecurityDynamicConfiguration<TenantV7> 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
@@ -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
@@ -297,7 +297,7 @@ public SecurityDynamicConfiguration<NewType> parseJson(CType<NewType> ctype, Str
         public SecurityDynamicConfiguration<NewType> convert(SecurityDynamicConfiguration<OldType> oldConfig, CType<NewType> ctype) {
             SecurityDynamicConfiguration<NewType> newConfig = SecurityDynamicConfiguration.empty(ctype);
             newConfig.setAutoConvertedFrom(oldConfig);
-//            newConfig.setSeqNoPrimaryTerm();
+            // newConfig.setSeqNoPrimaryTerm();
 
             for (Map.Entry<String, OldType> 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
@@ -86,7 +86,6 @@ public static <T> SecurityDynamicConfiguration<T> empty(CType<T> ctype, int vers
         return result;
     }
 
-
     @JsonIgnore
     public boolean notEmpty() {
         return !centries.isEmpty();
@@ -157,11 +156,11 @@ public static <T> SecurityDynamicConfiguration<T> 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<T> deepClone() {
         try {
             if (ctype != null) {
-                SecurityDynamicConfiguration<T> result = fromJson(DefaultObjectMapper.writeValueAsString(this, false), ctype, version, seqNo, primaryTerm);
+                SecurityDynamicConfiguration<T> 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<T> result = (SecurityDynamicConfiguration<T>) fromJsonWithoutAutoConversion(DefaultObjectMapper.writeValueAsString(this, false), ctypeUnsafe, version, seqNo, primaryTerm);
+                SecurityDynamicConfiguration<T> result = (SecurityDynamicConfiguration<T>) fromJsonWithoutAutoConversion(
+                    DefaultObjectMapper.writeValueAsString(this, false),
+                    ctypeUnsafe,
+                    version,
+                    seqNo,
+                    primaryTerm
+                );
                 return result;
             }
         } catch (Exception e) {
@@ -439,12 +450,24 @@ public SecurityDynamicConfiguration<T> deepClone() {
     public SecurityDynamicConfiguration<T> deepCloneWithRedaction() {
         try {
             if (ctype != null) {
-                SecurityDynamicConfiguration<T> result = fromJson(DefaultObjectMapper.writeValueAsStringAndRedactSensitive(this), ctype, version, seqNo, primaryTerm);
+                SecurityDynamicConfiguration<T> 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<T> result = (SecurityDynamicConfiguration<T>) fromJsonWithoutAutoConversion(DefaultObjectMapper.writeValueAsStringAndRedactSensitive(this), ctypeUnsafe, version, seqNo, primaryTerm);
+                SecurityDynamicConfiguration<T> result = (SecurityDynamicConfiguration<T>) 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
@@ -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/...test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java b/...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
@@ -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();