From e9d3f263a8cd8102f23d869faaabc11533a69a1e Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 22 Nov 2023 11:07:02 -0500 Subject: [PATCH] Switch UserInjectorPlugin to use reliable per-instance settings (#3760) ### Description Fix flaky tests in TransportUserInjectorIntegTest by removing reliance on a static variable that makes the test unpredictable. This instead adds a setting in the UserInjectorPlugin and relies on each node that has the plugin installed to supply this setting when instantiating the node. * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Test Fix ### Issues Resolved - Fixes https://github.com/opensearch-project/security/issues/3729 ### Check List - [ ] New functionality includes testing - [ ] New functionality has been documented - [ ] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). Signed-off-by: Craig Perkins --- .../TransportUserInjectorIntegTest.java | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java b/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java index b13e5fbb20..decd886e15 100644 --- a/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java +++ b/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java @@ -14,6 +14,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; +import java.util.List; import java.util.function.Supplier; import com.google.common.collect.Lists; @@ -26,7 +27,9 @@ import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; +import org.opensearch.core.common.Strings; import org.opensearch.core.common.io.stream.NamedWriteableRegistry; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; @@ -47,9 +50,10 @@ public class TransportUserInjectorIntegTest extends SingleClusterTest { + public static final String TEST_INJECTED_USER = "test_injected_user"; + public static class UserInjectorPlugin extends Plugin implements ActionPlugin { Settings settings; - public static String injectedUser = null; public UserInjectorPlugin(final Settings settings, final Path configPath) { this.settings = settings; @@ -69,17 +73,24 @@ public Collection createComponents( IndexNameExpressionResolver indexNameExpressionResolver, Supplier repositoriesServiceSupplier ) { - if (injectedUser != null) threadPool.getThreadContext() - .putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUser); + if (!Strings.isNullOrEmpty(settings.get(TEST_INJECTED_USER))) threadPool.getThreadContext() + .putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, settings.get(TEST_INJECTED_USER)); return new ArrayList<>(); } + + @Override + public List> getSettings() { + List> settings = new ArrayList>(); + settings.add(Setting.simpleString(TEST_INJECTED_USER, Setting.Property.NodeScope, Setting.Property.Filtered)); + return settings; + } } @Test public void testSecurityUserInjection() throws Exception { final Settings clusterNodeSettings = Settings.builder().put(ConfigConstants.SECURITY_UNSUPPORTED_INJECT_USER_ENABLED, true).build(); setup(clusterNodeSettings, new DynamicSecurityConfig().setSecurityRolesMapping("roles_transport_inject_user.yml"), Settings.EMPTY); - final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) + final Settings.Builder tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) .put(minimumSecuritySettings(Settings.EMPTY).get(0)) .put("cluster.name", clusterInfo.clustername) .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data") @@ -89,14 +100,13 @@ public void testSecurityUserInjection() throws Exception { .put("discovery.initial_state_timeout", "8s") .put("plugins.security.allow_default_init_securityindex", "true") .put(ConfigConstants.SECURITY_UNSUPPORTED_INJECT_USER_ENABLED, true) - .putList("discovery.zen.ping.unicast.hosts", clusterInfo.nodeHost + ":" + clusterInfo.nodePort) - .build(); + .putList("discovery.zen.ping.unicast.hosts", clusterInfo.nodeHost + ":" + clusterInfo.nodePort); // 1. without user injection try ( Node node = new PluginAwareNode( false, - tcSettings, + tcSettings.build(), Lists.newArrayList(Netty4ModulePlugin.class, OpenSearchSecurityPlugin.class, UserInjectorPlugin.class) ).start() ) { @@ -106,12 +116,11 @@ public void testSecurityUserInjection() throws Exception { } // 2. with invalid backend roles - UserInjectorPlugin.injectedUser = "ttt|kkk"; Exception exception = null; try ( Node node = new PluginAwareNode( false, - tcSettings, + tcSettings.put(TEST_INJECTED_USER, "ttt|kkk").build(), Lists.newArrayList(Netty4ModulePlugin.class, OpenSearchSecurityPlugin.class, UserInjectorPlugin.class) ).start() ) { @@ -126,11 +135,10 @@ public void testSecurityUserInjection() throws Exception { } // 3. with valid backend roles for injected user - UserInjectorPlugin.injectedUser = "injectedadmin|injecttest"; try ( Node node = new PluginAwareNode( false, - tcSettings, + tcSettings.put(TEST_INJECTED_USER, "injectedadmin|injecttest").build(), Lists.newArrayList(Netty4ModulePlugin.class, OpenSearchSecurityPlugin.class, UserInjectorPlugin.class) ).start() ) { @@ -146,7 +154,7 @@ public void testSecurityUserInjectionWithConfigDisabled() throws Exception { .put(ConfigConstants.SECURITY_UNSUPPORTED_INJECT_USER_ENABLED, false) .build(); setup(clusterNodeSettings, new DynamicSecurityConfig().setSecurityRolesMapping("roles_transport_inject_user.yml"), Settings.EMPTY); - final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) + final Settings.Builder tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) .put(minimumSecuritySettings(Settings.EMPTY).get(0)) .put("cluster.name", clusterInfo.clustername) .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data") @@ -156,14 +164,13 @@ public void testSecurityUserInjectionWithConfigDisabled() throws Exception { .put("discovery.initial_state_timeout", "8s") .put("plugins.security.allow_default_init_securityindex", "true") .put(ConfigConstants.SECURITY_UNSUPPORTED_INJECT_USER_ENABLED, false) - .putList("discovery.zen.ping.unicast.hosts", clusterInfo.nodeHost + ":" + clusterInfo.nodePort) - .build(); + .putList("discovery.zen.ping.unicast.hosts", clusterInfo.nodeHost + ":" + clusterInfo.nodePort); // 1. without user injection try ( Node node = new PluginAwareNode( false, - tcSettings, + tcSettings.build(), Lists.newArrayList(Netty4ModulePlugin.class, OpenSearchSecurityPlugin.class, UserInjectorPlugin.class) ).start() ) { @@ -173,11 +180,10 @@ public void testSecurityUserInjectionWithConfigDisabled() throws Exception { } // with invalid backend roles - UserInjectorPlugin.injectedUser = "ttt|kkk"; try ( Node node = new PluginAwareNode( false, - tcSettings, + tcSettings.put(TEST_INJECTED_USER, "ttt|kkk").build(), Lists.newArrayList(Netty4ModulePlugin.class, OpenSearchSecurityPlugin.class, UserInjectorPlugin.class) ).start() ) {