[Backport 2.x] Check for and perform upgrades on security configurati…

…ons (opensearch-project#4251)

Co-authored-by: Peter Nied <[email protected]>

src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java

@@ -11,17 +11,21 @@
 
 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;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
+import org.opensearch.test.framework.TestSecurityConfig.User;
 import org.opensearch.test.framework.cluster.ClusterManager;
 import org.opensearch.test.framework.cluster.LocalCluster;
 import org.opensearch.test.framework.cluster.TestRestClient;
@@ -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<String, Object> 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<String> extractFieldNames(final JsonNode json) {
+        final var set = new HashSet<String>();
+        json.fieldNames().forEachRemaining(set::add);
+        return set;
+    }
 }

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'

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();

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<SecurityConfiguration> patchEntity(
         );
     }
 
-    protected final ValidationResult<SecurityConfiguration> patchEntities(
+    protected ValidationResult<SecurityConfiguration> patchEntities(
         final RestRequest request,
         final JsonNode patchContent,
         final SecurityConfiguration securityConfiguration
@@ -336,7 +339,7 @@ final void saveOrUpdateConfiguration(
         final SecurityDynamicConfiguration<?> configuration,
         final OnSucessActionListener<IndexResponse> 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<SecurityConfiguration> loadConfiguration(final
         );
     }
 
-    protected final ValidationResult<SecurityDynamicConfiguration<?>> loadConfiguration(
+    protected ValidationResult<SecurityDynamicConfiguration<?>> 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<IndexResponse> 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<IndexResponse> 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<Response> implements ActionListener<Response> {