From d79563aabc6d2977a9512e8c92d5ffd792944bfa Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 7 Jan 2025 14:18:17 -0500 Subject: [PATCH 01/17] Refactor IdentityAwarePlugin interface to be assigned a client for executing actions Signed-off-by: Craig Perkins --- .../identity/shiro/ShiroIdentityPlugin.java | 9 ++- .../identity/shiro/ShiroPluginSubject.java | 49 ---------------- .../opensearch/identity/IdentityService.java | 12 ++-- .../opensearch/identity/PluginSubject.java | 19 ------ .../java/org/opensearch/identity/Subject.java | 8 --- .../identity/noop/NoopIdentityPlugin.java | 14 ++--- .../identity/noop/NoopPluginSubject.java | 49 ---------------- .../identity/noop/RunAsSystemClient.java | 56 ++++++++++++++++++ .../main/java/org/opensearch/node/Node.java | 26 ++++----- .../plugins/IdentityAwarePlugin.java | 6 +- .../opensearch/plugins/IdentityPlugin.java | 4 +- .../opensearch/plugins/PluginsService.java | 4 ++ .../opensearch/action/ActionModuleTests.java | 5 +- .../bootstrap/IdentityPluginTests.java | 16 +++-- .../extensions/ExtensionsManagerTests.java | 3 +- .../rest/ExtensionRestRequestTests.java | 4 +- .../RestInitializeExtensionActionTests.java | 10 +++- .../rest/RestSendToExtensionActionTests.java | 7 ++- .../identity/noop/NoopPluginSubjectTests.java | 58 ------------------- 19 files changed, 126 insertions(+), 233 deletions(-) delete mode 100644 plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroPluginSubject.java delete mode 100644 server/src/main/java/org/opensearch/identity/PluginSubject.java delete mode 100644 server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java create mode 100644 server/src/main/java/org/opensearch/identity/noop/RunAsSystemClient.java delete mode 100644 server/src/test/java/org/opensearch/identity/noop/NoopPluginSubjectTests.java diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java index 2da788242a745..5fc93a144ec88 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java @@ -13,6 +13,7 @@ import org.apache.shiro.SecurityUtils; import org.apache.shiro.mgt.SecurityManager; import org.opensearch.client.Client; +import org.opensearch.client.FilterClient; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.service.ClusterService; @@ -23,8 +24,8 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; import org.opensearch.env.NodeEnvironment; -import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; +import org.opensearch.identity.noop.RunAsSystemClient; import org.opensearch.identity.tokens.AuthToken; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.ActionPlugin; @@ -54,6 +55,7 @@ public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin, private final ShiroTokenManager authTokenHandler; private ThreadPool threadPool; + private Client client; /** * Create a new instance of the Shiro Identity Plugin @@ -83,6 +85,7 @@ public Collection createComponents( Supplier repositoriesServiceSupplier ) { this.threadPool = threadPool; + this.client = client; return Collections.emptyList(); } @@ -138,7 +141,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } } - public PluginSubject getPluginSubject(Plugin plugin) { - return new ShiroPluginSubject(threadPool); + public FilterClient getRunAsClient(Plugin plugin) { + return new RunAsSystemClient(client); } } diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroPluginSubject.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroPluginSubject.java deleted file mode 100644 index 31dde34f447d4..0000000000000 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroPluginSubject.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.identity.shiro; - -import org.opensearch.common.annotation.ExperimentalApi; -import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.identity.NamedPrincipal; -import org.opensearch.identity.PluginSubject; -import org.opensearch.threadpool.ThreadPool; - -import java.security.Principal; -import java.util.concurrent.Callable; - -/** - * Implementation of subject that is always authenticated - *

- * This class and related classes in this package will not return nulls or fail permissions checks - * - * This class is used by the ShiroIdentityPlugin to initialize IdentityAwarePlugins - * - * @opensearch.experimental - */ -@ExperimentalApi -public class ShiroPluginSubject implements PluginSubject { - private final ThreadPool threadPool; - - ShiroPluginSubject(ThreadPool threadPool) { - super(); - this.threadPool = threadPool; - } - - @Override - public Principal getPrincipal() { - return NamedPrincipal.UNAUTHENTICATED; - } - - @Override - public T runAs(Callable callable) throws Exception { - try (ThreadContext.StoredContext ctx = threadPool.getThreadContext().stashContext()) { - return callable.call(); - } - } -} diff --git a/server/src/main/java/org/opensearch/identity/IdentityService.java b/server/src/main/java/org/opensearch/identity/IdentityService.java index 33066fae5a80d..bbe2aaaa2eeca 100644 --- a/server/src/main/java/org/opensearch/identity/IdentityService.java +++ b/server/src/main/java/org/opensearch/identity/IdentityService.java @@ -8,6 +8,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.OpenSearchException; +import org.opensearch.client.Client; import org.opensearch.common.annotation.InternalApi; import org.opensearch.common.settings.Settings; import org.opensearch.identity.noop.NoopIdentityPlugin; @@ -15,7 +16,6 @@ import org.opensearch.plugins.IdentityAwarePlugin; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.plugins.Plugin; -import org.opensearch.threadpool.ThreadPool; import java.util.List; import java.util.stream.Collectors; @@ -30,14 +30,16 @@ public class IdentityService { private static final Logger log = LogManager.getLogger(IdentityService.class); private final Settings settings; + private final Client client; private final IdentityPlugin identityPlugin; - public IdentityService(final Settings settings, final ThreadPool threadPool, final List identityPlugins) { + public IdentityService(final Settings settings, final Client client, final List identityPlugins) { this.settings = settings; + this.client = client; if (identityPlugins.size() == 0) { log.debug("Identity plugins size is 0"); - identityPlugin = new NoopIdentityPlugin(threadPool); + identityPlugin = new NoopIdentityPlugin(client); } else if (identityPlugins.size() == 1) { log.debug("Identity plugins size is 1"); identityPlugin = identityPlugins.get(0); @@ -66,8 +68,8 @@ public TokenManager getTokenManager() { public void initializeIdentityAwarePlugins(final List identityAwarePlugins) { if (identityAwarePlugins != null) { for (IdentityAwarePlugin plugin : identityAwarePlugins) { - PluginSubject pluginSubject = identityPlugin.getPluginSubject((Plugin) plugin); - plugin.assignSubject(pluginSubject); + Client client = identityPlugin.getRunAsClient((Plugin) plugin); + plugin.assignRunAsClient(client); } } } diff --git a/server/src/main/java/org/opensearch/identity/PluginSubject.java b/server/src/main/java/org/opensearch/identity/PluginSubject.java deleted file mode 100644 index 3ea42182d3fc3..0000000000000 --- a/server/src/main/java/org/opensearch/identity/PluginSubject.java +++ /dev/null @@ -1,19 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.identity; - -import org.opensearch.common.annotation.ExperimentalApi; - -/** - * Similar to {@link Subject}, but represents a plugin executing actions - * - * @opensearch.experimental - */ -@ExperimentalApi -public interface PluginSubject extends Subject {} diff --git a/server/src/main/java/org/opensearch/identity/Subject.java b/server/src/main/java/org/opensearch/identity/Subject.java index 0fb0e53848d80..75bc3460c1be4 100644 --- a/server/src/main/java/org/opensearch/identity/Subject.java +++ b/server/src/main/java/org/opensearch/identity/Subject.java @@ -8,7 +8,6 @@ import org.opensearch.common.annotation.ExperimentalApi; import java.security.Principal; -import java.util.concurrent.Callable; /** * An individual, process, or device that causes information to flow among objects or change to the system state. @@ -22,11 +21,4 @@ public interface Subject { * Get the application-wide uniquely identifying principal * */ Principal getPrincipal(); - - /** - * runAs allows the caller to run a callable function as this subject - */ - default T runAs(Callable callable) throws Exception { - return callable.call(); - }; } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java b/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java index 6279388c76f96..8ae9b145f7020 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java @@ -8,12 +8,12 @@ package org.opensearch.identity.noop; -import org.opensearch.identity.PluginSubject; +import org.opensearch.client.Client; +import org.opensearch.client.FilterClient; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.plugins.Plugin; -import org.opensearch.threadpool.ThreadPool; /** * Implementation of identity plugin that does not enforce authentication or authorization @@ -24,10 +24,10 @@ */ public class NoopIdentityPlugin implements IdentityPlugin { - private final ThreadPool threadPool; + private final Client client; - public NoopIdentityPlugin(ThreadPool threadPool) { - this.threadPool = threadPool; + public NoopIdentityPlugin(Client client) { + this.client = client; } /** @@ -49,7 +49,7 @@ public TokenManager getTokenManager() { } @Override - public PluginSubject getPluginSubject(Plugin plugin) { - return new NoopPluginSubject(threadPool); + public FilterClient getRunAsClient(Plugin plugin) { + return new RunAsSystemClient(client); } } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java b/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java deleted file mode 100644 index 20e075276f317..0000000000000 --- a/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.identity.noop; - -import org.opensearch.common.annotation.InternalApi; -import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.identity.NamedPrincipal; -import org.opensearch.identity.PluginSubject; -import org.opensearch.threadpool.ThreadPool; - -import java.security.Principal; -import java.util.concurrent.Callable; - -/** - * Implementation of subject that is always authenticated - *

- * This class and related classes in this package will not return nulls or fail permissions checks - * - * This class is used by the NoopIdentityPlugin to initialize IdentityAwarePlugins - * - * @opensearch.internal - */ -@InternalApi -public class NoopPluginSubject implements PluginSubject { - private final ThreadPool threadPool; - - NoopPluginSubject(ThreadPool threadPool) { - super(); - this.threadPool = threadPool; - } - - @Override - public Principal getPrincipal() { - return NamedPrincipal.UNAUTHENTICATED; - } - - @Override - public T runAs(Callable callable) throws Exception { - try (ThreadContext.StoredContext ctx = threadPool.getThreadContext().stashContext()) { - return callable.call(); - } - } -} diff --git a/server/src/main/java/org/opensearch/identity/noop/RunAsSystemClient.java b/server/src/main/java/org/opensearch/identity/noop/RunAsSystemClient.java new file mode 100644 index 0000000000000..aae251c6cb3be --- /dev/null +++ b/server/src/main/java/org/opensearch/identity/noop/RunAsSystemClient.java @@ -0,0 +1,56 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.identity.noop; + +import org.opensearch.action.ActionRequest; +import org.opensearch.action.ActionType; +import org.opensearch.client.Client; +import org.opensearch.client.FilterClient; +import org.opensearch.common.annotation.InternalApi; +import org.opensearch.common.util.concurrent.ThreadContext; +import org.opensearch.core.action.ActionListener; +import org.opensearch.core.action.ActionResponse; + +/** + * Implementation of client that will run transport actions in a stashed context + *

+ * This class and related classes in this package will not return nulls or fail permissions checks + * + * This class is used by the NoopIdentityPlugin to initialize IdentityAwarePlugins + * + * @opensearch.internal + */ +@InternalApi +public class RunAsSystemClient extends FilterClient { + public RunAsSystemClient(Client delegate) { + super(delegate); + } + + @Override + protected void doExecute( + ActionType action, + Request request, + ActionListener actionListener + ) { + ThreadContext threadContext = threadPool().getThreadContext(); + + try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { + + ActionListener wrappedListener = ActionListener.wrap(r -> { + ctx.restore(); + actionListener.onResponse(r); + }, e -> { + ctx.restore(); + actionListener.onFailure(e); + }); + + super.doExecute(action, request, wrappedListener); + } + } +} diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 704a23890b07a..2c48a78240cb4 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -588,19 +588,6 @@ protected Node( runnableTaskListener = new AtomicReference<>(); final ThreadPool threadPool = new ThreadPool(settings, runnableTaskListener, executorBuilders.toArray(new ExecutorBuilder[0])); - final IdentityService identityService = new IdentityService(settings, threadPool, identityPlugins); - - if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - final List extensionAwarePlugins = pluginsService.filterPlugins(ExtensionAwarePlugin.class); - Set> additionalSettings = new HashSet<>(); - for (ExtensionAwarePlugin extAwarePlugin : extensionAwarePlugins) { - additionalSettings.addAll(extAwarePlugin.getExtensionSettings()); - } - this.extensionsManager = new ExtensionsManager(additionalSettings, identityService); - } else { - this.extensionsManager = new NoopExtensionsManager(identityService); - } - final SetOnce repositoriesServiceReference = new SetOnce<>(); final RemoteStoreNodeService remoteStoreNodeService = new RemoteStoreNodeService(repositoriesServiceReference::get, threadPool); localNodeFactory = new LocalNodeFactory(settings, nodeEnvironment.nodeId(), remoteStoreNodeService); @@ -624,6 +611,19 @@ protected Node( } client = new NodeClient(settings, threadPool); + final IdentityService identityService = new IdentityService(settings, client, identityPlugins); + + if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { + final List extensionAwarePlugins = pluginsService.filterPlugins(ExtensionAwarePlugin.class); + Set> extAdditionalSettings = new HashSet<>(); + for (ExtensionAwarePlugin extAwarePlugin : extensionAwarePlugins) { + extAdditionalSettings.addAll(extAwarePlugin.getExtensionSettings()); + } + this.extensionsManager = new ExtensionsManager(extAdditionalSettings, identityService); + } else { + this.extensionsManager = new NoopExtensionsManager(identityService); + } + final ScriptModule scriptModule = new ScriptModule(settings, pluginsService.filterPlugins(ScriptPlugin.class)); final ScriptService scriptService = newScriptService(settings, scriptModule.engines, scriptModule.contexts); AnalysisModule analysisModule = new AnalysisModule(this.environment, pluginsService.filterPlugins(AnalysisPlugin.class)); diff --git a/server/src/main/java/org/opensearch/plugins/IdentityAwarePlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityAwarePlugin.java index b19dcfe5544ec..9acc162c1fdf8 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityAwarePlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityAwarePlugin.java @@ -8,8 +8,8 @@ package org.opensearch.plugins; +import org.opensearch.client.Client; import org.opensearch.common.annotation.ExperimentalApi; -import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; /** @@ -27,8 +27,8 @@ public interface IdentityAwarePlugin { /** * Passes necessary classes for this plugin to operate as an IdentityAwarePlugin * - * @param pluginSubject A subject for running transport actions in the plugin context for system index + * @param pluginClient A subject for running transport actions in the plugin context for system index * interaction */ - default void assignSubject(PluginSubject pluginSubject) {} + default void assignRunAsClient(Client pluginClient) {} } diff --git a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java index b40af14231fb9..70aab42f62651 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java @@ -8,8 +8,8 @@ package org.opensearch.plugins; +import org.opensearch.client.Client; import org.opensearch.common.annotation.ExperimentalApi; -import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; @@ -41,5 +41,5 @@ public interface IdentityPlugin { * @param plugin The corresponding plugin * @return Subject corresponding to the plugin */ - PluginSubject getPluginSubject(Plugin plugin); + Client getRunAsClient(Plugin plugin); } diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index 9bc1f1334122e..90a7bdc14ff42 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -833,4 +833,8 @@ private String signatureMessage(final Class clazz) { public List filterPlugins(Class type) { return plugins.stream().filter(x -> type.isAssignableFrom(x.v2().getClass())).map(p -> ((T) p.v2())).collect(Collectors.toList()); } + + public List filterPluginInfos(Class type) { + return plugins.stream().filter(x -> type.isAssignableFrom(x.v2().getClass())).map(Tuple::v1).collect(Collectors.toList()); + } } diff --git a/server/src/test/java/org/opensearch/action/ActionModuleTests.java b/server/src/test/java/org/opensearch/action/ActionModuleTests.java index 6b8951dd43d11..d2d37650881e7 100644 --- a/server/src/test/java/org/opensearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/opensearch/action/ActionModuleTests.java @@ -36,6 +36,7 @@ import org.opensearch.action.main.TransportMainAction; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.TransportAction; +import org.opensearch.client.Client; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.node.DiscoveryNodes; @@ -143,8 +144,8 @@ public void testSetupRestHandlerContainsKnownBuiltin() throws IOException { null, usageService, null, - new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()), - new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of())) + new IdentityService(Settings.EMPTY, mock(Client.class), new ArrayList<>()), + new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(Client.class), List.of())) ); actionModule.initRestHandlers(null); // At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail diff --git a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java index d7b9f5917c366..73f0cc8d5689a 100644 --- a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java +++ b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java @@ -9,6 +9,7 @@ package org.opensearch.bootstrap; import org.opensearch.OpenSearchException; +import org.opensearch.client.Client; import org.opensearch.common.settings.Settings; import org.opensearch.identity.IdentityService; import org.opensearch.identity.noop.NoopIdentityPlugin; @@ -21,26 +22,29 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; +import static org.mockito.Mockito.mock; public class IdentityPluginTests extends OpenSearchTestCase { public void testSingleIdentityPluginSucceeds() { + Client client = mock(Client.class); TestThreadPool threadPool = new TestThreadPool(getTestName()); - IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(threadPool); + IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(client); List pluginList1 = List.of(identityPlugin1); - IdentityService identityService1 = new IdentityService(Settings.EMPTY, threadPool, pluginList1); + IdentityService identityService1 = new IdentityService(Settings.EMPTY, client, pluginList1); assertTrue(identityService1.getCurrentSubject().getPrincipal().getName().equalsIgnoreCase("Unauthenticated")); assertThat(identityService1.getTokenManager(), is(instanceOf(NoopTokenManager.class))); terminate(threadPool); } public void testMultipleIdentityPluginsFail() { + Client client = mock(Client.class); TestThreadPool threadPool = new TestThreadPool(getTestName()); - IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(threadPool); - IdentityPlugin identityPlugin2 = new NoopIdentityPlugin(threadPool); - IdentityPlugin identityPlugin3 = new NoopIdentityPlugin(threadPool); + IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(client); + IdentityPlugin identityPlugin2 = new NoopIdentityPlugin(client); + IdentityPlugin identityPlugin3 = new NoopIdentityPlugin(client); List pluginList = List.of(identityPlugin1, identityPlugin2, identityPlugin3); - Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, threadPool, pluginList)); + Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, client, pluginList)); assert (ex.getMessage().contains("Multiple identity plugins are not supported,")); terminate(threadPool); } diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index bf1d52b49cb1f..70671b702d019 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -15,6 +15,7 @@ import org.opensearch.action.ActionModule; import org.opensearch.action.ActionModule.DynamicActionRegistry; import org.opensearch.action.admin.cluster.state.ClusterStateResponse; +import org.opensearch.client.Client; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.ClusterSettingsResponse; import org.opensearch.cluster.metadata.IndexMetadata; @@ -170,7 +171,7 @@ public List> getExtensionSettings() { Collections.emptyList() ); client = new NoOpNodeClient(this.getTestName()); - identityService = new IdentityService(Settings.EMPTY, threadPool, List.of()); + identityService = new IdentityService(Settings.EMPTY, mock(Client.class), List.of()); } @Override diff --git a/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java b/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java index 7d9ebe1d1e26a..677486b769694 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java @@ -9,6 +9,7 @@ package org.opensearch.extensions.rest; import org.opensearch.OpenSearchParseException; +import org.opensearch.client.Client; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.settings.Settings; import org.opensearch.core.common.bytes.BytesArray; @@ -29,7 +30,6 @@ import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestRequest.Method; import org.opensearch.test.OpenSearchTestCase; -import org.opensearch.threadpool.ThreadPool; import java.nio.charset.StandardCharsets; import java.security.Principal; @@ -74,7 +74,7 @@ public void setUp() throws Exception { userPrincipal = () -> "user1"; expectedHttpVersion = HttpRequest.HttpVersion.HTTP_1_1; extensionTokenProcessor = "placeholder_extension_token_processor"; - identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()); + identityService = new IdentityService(Settings.EMPTY, mock(Client.class), List.of()); TokenManager tokenManager = identityService.getTokenManager(); Subject subject = this.identityService.getCurrentSubject(); OnBehalfOfClaims claims = new OnBehalfOfClaims("testID", subject.getPrincipal().getName()); diff --git a/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java b/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java index ac818c3bb4a7b..a725017de6b2a 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java @@ -9,6 +9,7 @@ package org.opensearch.extensions.rest; import org.opensearch.Version; +import org.opensearch.client.Client; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.common.network.NetworkService; import org.opensearch.common.settings.Setting; @@ -121,7 +122,10 @@ public void testRestInitializeExtensionActionResponse() throws Exception { } public void testRestInitializeExtensionActionFailure() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, threadPool, List.of())); + ExtensionsManager extensionsManager = new ExtensionsManager( + Set.of(), + new IdentityService(Settings.EMPTY, mock(Client.class), List.of()) + ); RestInitializeExtensionAction restInitializeExtensionAction = new RestInitializeExtensionAction(extensionsManager); final String content = "{\"name\":\"ad-extension\",\"uniqueId\":\"\",\"hostAddress\":\"127.0.0.1\"," @@ -156,7 +160,7 @@ public void testRestInitializeExtensionActionResponseWithAdditionalSettings() th ); ExtensionsManager extensionsManager = new ExtensionsManager( Set.of(boolSetting, stringSetting, intSetting, listSetting), - new IdentityService(Settings.EMPTY, threadPool, List.of()) + new IdentityService(Settings.EMPTY, mock(Client.class), List.of()) ); ExtensionsManager spy = spy(extensionsManager); @@ -206,7 +210,7 @@ public void testRestInitializeExtensionActionResponseWithAdditionalSettingsUsing ); ExtensionsManager extensionsManager = new ExtensionsManager( Set.of(boolSetting, stringSetting, intSetting, listSetting), - new IdentityService(Settings.EMPTY, threadPool, List.of()) + new IdentityService(Settings.EMPTY, mock(Client.class), List.of()) ); ExtensionsManager spy = spy(extensionsManager); diff --git a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java index e9c910ea361fb..a0a3bdcf3b818 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java @@ -14,6 +14,7 @@ import org.opensearch.action.admin.cluster.health.ClusterHealthAction; import org.opensearch.action.admin.cluster.health.TransportClusterHealthAction; import org.opensearch.action.support.ActionFilters; +import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.common.network.NetworkService; @@ -122,10 +123,10 @@ public void setup() throws Exception { null, usageService, null, - new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()), - new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of())) + new IdentityService(Settings.EMPTY, mock(Client.class), new ArrayList<>()), + new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(Client.class), List.of())) ); - identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()); + identityService = new IdentityService(Settings.EMPTY, mock(Client.class), new ArrayList<>()); dynamicActionRegistry = actionModule.getDynamicActionRegistry(); } diff --git a/server/src/test/java/org/opensearch/identity/noop/NoopPluginSubjectTests.java b/server/src/test/java/org/opensearch/identity/noop/NoopPluginSubjectTests.java deleted file mode 100644 index 79c26a7eb790d..0000000000000 --- a/server/src/test/java/org/opensearch/identity/noop/NoopPluginSubjectTests.java +++ /dev/null @@ -1,58 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.identity.noop; - -import org.opensearch.common.settings.Settings; -import org.opensearch.identity.IdentityService; -import org.opensearch.identity.NamedPrincipal; -import org.opensearch.identity.PluginSubject; -import org.opensearch.plugins.IdentityAwarePlugin; -import org.opensearch.plugins.Plugin; -import org.opensearch.test.OpenSearchTestCase; -import org.opensearch.threadpool.TestThreadPool; -import org.opensearch.threadpool.ThreadPool; - -import java.util.List; - -import static org.hamcrest.Matchers.equalTo; - -public class NoopPluginSubjectTests extends OpenSearchTestCase { - public static class TestPlugin extends Plugin implements IdentityAwarePlugin { - private PluginSubject subject; - - @Override - public void assignSubject(PluginSubject subject) { - this.subject = subject; - } - - public PluginSubject getSubject() { - return subject; - } - } - - public void testInitializeIdentityAwarePlugin() throws Exception { - ThreadPool threadPool = new TestThreadPool(getTestName()); - IdentityService identityService = new IdentityService(Settings.EMPTY, threadPool, List.of()); - - TestPlugin testPlugin = new TestPlugin(); - identityService.initializeIdentityAwarePlugins(List.of(testPlugin)); - - PluginSubject testPluginSubject = new NoopPluginSubject(threadPool); - assertThat(testPlugin.getSubject().getPrincipal().getName(), equalTo(NamedPrincipal.UNAUTHENTICATED.getName())); - assertThat(testPluginSubject.getPrincipal().getName(), equalTo(NamedPrincipal.UNAUTHENTICATED.getName())); - threadPool.getThreadContext().putHeader("test_header", "foo"); - assertThat(threadPool.getThreadContext().getHeader("test_header"), equalTo("foo")); - testPluginSubject.runAs(() -> { - assertNull(threadPool.getThreadContext().getHeader("test_header")); - return null; - }); - assertThat(threadPool.getThreadContext().getHeader("test_header"), equalTo("foo")); - terminate(threadPool); - } -} From 2765e8838f39b7326a453eafb48fc23b00267930 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 7 Jan 2025 14:31:40 -0500 Subject: [PATCH 02/17] Add to CHANGELOG Signed-off-by: Craig Perkins --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0efb53beb6e31..3df20a226b324 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Indexed IP field supports `terms_query` with more than 1025 IP masks [#16391](https://github.com/opensearch-project/OpenSearch/pull/16391) - Make entries for dependencies from server/build.gradle to gradle version catalog ([#16707](https://github.com/opensearch-project/OpenSearch/pull/16707)) - Allow extended plugins to be optional ([#16909](https://github.com/opensearch-project/OpenSearch/pull/16909)) +- Refactor IdentityAwarePlugin interface to be assigned a client for executing actions ([#16976](https://github.com/opensearch-project/OpenSearch/pull/16976)) ### Deprecated - Performing update operation with default pipeline or final pipeline is deprecated ([#16712](https://github.com/opensearch-project/OpenSearch/pull/16712)) From be4b7a5576b674f3d386f8d8fdf2a5a324d9dd66 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 7 Jan 2025 14:48:19 -0500 Subject: [PATCH 03/17] Fix shiro test Signed-off-by: Craig Perkins --- .../identity/shiro/ShiroIdentityPluginTests.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java index a15538e48bd66..2b40fda145aa7 100644 --- a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java +++ b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java @@ -9,11 +9,11 @@ package org.opensearch.identity.shiro; import org.opensearch.OpenSearchException; +import org.opensearch.client.Client; import org.opensearch.common.settings.Settings; import org.opensearch.identity.IdentityService; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.test.OpenSearchTestCase; -import org.opensearch.threadpool.ThreadPool; import java.util.List; @@ -28,7 +28,7 @@ public class ShiroIdentityPluginTests extends OpenSearchTestCase { public void testSingleIdentityPluginSucceeds() { IdentityPlugin identityPlugin1 = new ShiroIdentityPlugin(Settings.EMPTY); List pluginList1 = List.of(identityPlugin1); - IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList1); + IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(Client.class), pluginList1); assertThat(identityService1.getTokenManager(), is(instanceOf(ShiroTokenManager.class))); } @@ -37,10 +37,7 @@ public void testMultipleIdentityPluginsFail() { IdentityPlugin identityPlugin2 = new ShiroIdentityPlugin(Settings.EMPTY); IdentityPlugin identityPlugin3 = new ShiroIdentityPlugin(Settings.EMPTY); List pluginList = List.of(identityPlugin1, identityPlugin2, identityPlugin3); - Exception ex = assertThrows( - OpenSearchException.class, - () -> new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList) - ); + Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, mock(Client.class), pluginList)); assert (ex.getMessage().contains("Multiple identity plugins are not supported,")); } From 7a20d21a2190657cc0f19e10dff5ed1f0fb6b6cc Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 7 Jan 2025 16:22:07 -0500 Subject: [PATCH 04/17] Add tests for RunAsSystemClient Signed-off-by: Craig Perkins --- .../identity/RunAsSystemClientTests.java | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java diff --git a/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java b/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java new file mode 100644 index 0000000000000..65b3b72243c2c --- /dev/null +++ b/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java @@ -0,0 +1,62 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.identity; + +import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; +import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; +import org.opensearch.client.Client; +import org.opensearch.core.action.ActionListener; +import org.opensearch.identity.noop.RunAsSystemClient; +import org.opensearch.test.OpenSearchSingleNodeTestCase; +import org.junit.Before; + +import static org.hamcrest.Matchers.equalTo; + +public class RunAsSystemClientTests extends OpenSearchSingleNodeTestCase { + @Before + public void setup() { + client().threadPool().getThreadContext().stashContext(); // start with fresh context + } + + public void testThatContextIsRestoredOnActionListenerResponse() throws Exception { + Client systemClient = new RunAsSystemClient(client()); + systemClient.threadPool().getThreadContext().putHeader("test_header", "foo"); + + systemClient.admin().cluster().health(new ClusterHealthRequest(), new ActionListener<>() { + @Override + public void onResponse(ClusterHealthResponse clusterHealthResponse) { + String testHeader = systemClient.threadPool().getThreadContext().getHeader("test_header"); + assertThat(testHeader, equalTo("foo")); + } + + @Override + public void onFailure(Exception e) { + fail("Expected cluster health action to succeed"); + } + }); + } + + public void testThatContextIsRestoredOnActionListenerFailure() throws Exception { + Client systemClient = new RunAsSystemClient(client()); + systemClient.threadPool().getThreadContext().putHeader("test_header", "bar"); + + systemClient.admin().cluster().health(new ClusterHealthRequest("dne"), new ActionListener<>() { + @Override + public void onResponse(ClusterHealthResponse clusterHealthResponse) { + fail("Expected cluster health action to fail"); + } + + @Override + public void onFailure(Exception e) { + String testHeader = systemClient.threadPool().getThreadContext().getHeader("test_header"); + assertThat(testHeader, equalTo("bar")); + } + }); + } +} From ad3fbb64fa08240ef21e7dc42e9778f3815effe9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 7 Jan 2025 18:06:23 -0500 Subject: [PATCH 05/17] Move instantiation of the NodeClient up slightly Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/node/Node.java | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 2c48a78240cb4..eb8c47b2eb945 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -588,6 +588,21 @@ protected Node( runnableTaskListener = new AtomicReference<>(); final ThreadPool threadPool = new ThreadPool(settings, runnableTaskListener, executorBuilders.toArray(new ExecutorBuilder[0])); + client = new NodeClient(settings, threadPool); + + final IdentityService identityService = new IdentityService(settings, client, identityPlugins); + + if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { + final List extensionAwarePlugins = pluginsService.filterPlugins(ExtensionAwarePlugin.class); + Set> additionalSettings = new HashSet<>(); + for (ExtensionAwarePlugin extAwarePlugin : extensionAwarePlugins) { + additionalSettings.addAll(extAwarePlugin.getExtensionSettings()); + } + this.extensionsManager = new ExtensionsManager(additionalSettings, identityService); + } else { + this.extensionsManager = new NoopExtensionsManager(identityService); + } + final SetOnce repositoriesServiceReference = new SetOnce<>(); final RemoteStoreNodeService remoteStoreNodeService = new RemoteStoreNodeService(repositoriesServiceReference::get, threadPool); localNodeFactory = new LocalNodeFactory(settings, nodeEnvironment.nodeId(), remoteStoreNodeService); @@ -609,20 +624,6 @@ protected Node( for (final ExecutorBuilder builder : threadPool.builders()) { additionalSettings.addAll(builder.getRegisteredSettings()); } - client = new NodeClient(settings, threadPool); - - final IdentityService identityService = new IdentityService(settings, client, identityPlugins); - - if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - final List extensionAwarePlugins = pluginsService.filterPlugins(ExtensionAwarePlugin.class); - Set> extAdditionalSettings = new HashSet<>(); - for (ExtensionAwarePlugin extAwarePlugin : extensionAwarePlugins) { - extAdditionalSettings.addAll(extAwarePlugin.getExtensionSettings()); - } - this.extensionsManager = new ExtensionsManager(extAdditionalSettings, identityService); - } else { - this.extensionsManager = new NoopExtensionsManager(identityService); - } final ScriptModule scriptModule = new ScriptModule(settings, pluginsService.filterPlugins(ScriptPlugin.class)); final ScriptService scriptService = newScriptService(settings, scriptModule.engines, scriptModule.contexts); From 9439a0ebaaac5369de7f295caf7e89d8434854c4 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 8 Jan 2025 10:24:52 -0500 Subject: [PATCH 06/17] Use general Client Signed-off-by: Craig Perkins --- .../org/opensearch/identity/shiro/ShiroIdentityPlugin.java | 3 +-- .../java/org/opensearch/identity/noop/NoopIdentityPlugin.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java index 5fc93a144ec88..f6e89d25ea548 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java @@ -13,7 +13,6 @@ import org.apache.shiro.SecurityUtils; import org.apache.shiro.mgt.SecurityManager; import org.opensearch.client.Client; -import org.opensearch.client.FilterClient; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.service.ClusterService; @@ -141,7 +140,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } } - public FilterClient getRunAsClient(Plugin plugin) { + public Client getRunAsClient(Plugin plugin) { return new RunAsSystemClient(client); } } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java b/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java index 8ae9b145f7020..7aa433e81e15b 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java @@ -9,7 +9,6 @@ package org.opensearch.identity.noop; import org.opensearch.client.Client; -import org.opensearch.client.FilterClient; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.IdentityPlugin; @@ -49,7 +48,7 @@ public TokenManager getTokenManager() { } @Override - public FilterClient getRunAsClient(Plugin plugin) { + public Client getRunAsClient(Plugin plugin) { return new RunAsSystemClient(client); } } From c8fab691694a18b94d919ba5d48c0dd777f5f971 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 13 Jan 2025 09:39:53 -0500 Subject: [PATCH 07/17] Respond to code review feedback Signed-off-by: Craig Perkins --- .../identity/shiro/ShiroIdentityPlugin.java | 2 +- .../identity/{noop => }/RunAsSystemClient.java | 16 +++------------- .../identity/noop/NoopIdentityPlugin.java | 1 + .../identity/RunAsSystemClientTests.java | 1 - 4 files changed, 5 insertions(+), 15 deletions(-) rename server/src/main/java/org/opensearch/identity/{noop => }/RunAsSystemClient.java (76%) diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java index f6e89d25ea548..4167c0efb6ff7 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java @@ -23,8 +23,8 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; import org.opensearch.env.NodeEnvironment; +import org.opensearch.identity.RunAsSystemClient; import org.opensearch.identity.Subject; -import org.opensearch.identity.noop.RunAsSystemClient; import org.opensearch.identity.tokens.AuthToken; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.ActionPlugin; diff --git a/server/src/main/java/org/opensearch/identity/noop/RunAsSystemClient.java b/server/src/main/java/org/opensearch/identity/RunAsSystemClient.java similarity index 76% rename from server/src/main/java/org/opensearch/identity/noop/RunAsSystemClient.java rename to server/src/main/java/org/opensearch/identity/RunAsSystemClient.java index aae251c6cb3be..da7b9a2d98a69 100644 --- a/server/src/main/java/org/opensearch/identity/noop/RunAsSystemClient.java +++ b/server/src/main/java/org/opensearch/identity/RunAsSystemClient.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.identity.noop; +package org.opensearch.identity; import org.opensearch.action.ActionRequest; import org.opensearch.action.ActionType; @@ -36,21 +36,11 @@ public RunAsSystemClient(Client delegate) { protected void doExecute( ActionType action, Request request, - ActionListener actionListener + ActionListener listener ) { ThreadContext threadContext = threadPool().getThreadContext(); - try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { - - ActionListener wrappedListener = ActionListener.wrap(r -> { - ctx.restore(); - actionListener.onResponse(r); - }, e -> { - ctx.restore(); - actionListener.onFailure(e); - }); - - super.doExecute(action, request, wrappedListener); + super.doExecute(action, request, ActionListener.runBefore(listener, ctx::restore)); } } } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java b/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java index 7aa433e81e15b..e08b1d2de7f49 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java @@ -9,6 +9,7 @@ package org.opensearch.identity.noop; import org.opensearch.client.Client; +import org.opensearch.identity.RunAsSystemClient; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.IdentityPlugin; diff --git a/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java b/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java index 65b3b72243c2c..2a7ce36cac3a3 100644 --- a/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java +++ b/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java @@ -12,7 +12,6 @@ import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; import org.opensearch.client.Client; import org.opensearch.core.action.ActionListener; -import org.opensearch.identity.noop.RunAsSystemClient; import org.opensearch.test.OpenSearchSingleNodeTestCase; import org.junit.Before; From 4a884f2562beb0566850785026f5ed234e12cb03 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 13 Jan 2025 14:19:36 -0500 Subject: [PATCH 08/17] Remove unused method Signed-off-by: Craig Perkins --- .../src/main/java/org/opensearch/plugins/PluginsService.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index 90a7bdc14ff42..9bc1f1334122e 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -833,8 +833,4 @@ private String signatureMessage(final Class clazz) { public List filterPlugins(Class type) { return plugins.stream().filter(x -> type.isAssignableFrom(x.v2().getClass())).map(p -> ((T) p.v2())).collect(Collectors.toList()); } - - public List filterPluginInfos(Class type) { - return plugins.stream().filter(x -> type.isAssignableFrom(x.v2().getClass())).map(Tuple::v1).collect(Collectors.toList()); - } } From 5f43e1793565996e6f92ed832672087866ee0f98 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 13 Jan 2025 17:01:16 -0500 Subject: [PATCH 09/17] Address review comments Signed-off-by: Craig Perkins --- .../java/org/opensearch/bootstrap/IdentityPluginTests.java | 3 +++ .../java/org/opensearch/extensions/ExtensionsManagerTests.java | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java index 73f0cc8d5689a..af3486fd5ab01 100644 --- a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java +++ b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java @@ -23,12 +23,14 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class IdentityPluginTests extends OpenSearchTestCase { public void testSingleIdentityPluginSucceeds() { Client client = mock(Client.class); TestThreadPool threadPool = new TestThreadPool(getTestName()); + when(client.threadPool()).thenReturn(threadPool); IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(client); List pluginList1 = List.of(identityPlugin1); IdentityService identityService1 = new IdentityService(Settings.EMPTY, client, pluginList1); @@ -40,6 +42,7 @@ public void testSingleIdentityPluginSucceeds() { public void testMultipleIdentityPluginsFail() { Client client = mock(Client.class); TestThreadPool threadPool = new TestThreadPool(getTestName()); + when(client.threadPool()).thenReturn(threadPool); IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(client); IdentityPlugin identityPlugin2 = new NoopIdentityPlugin(client); IdentityPlugin identityPlugin3 = new NoopIdentityPlugin(client); diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 70671b702d019..2d30ba8791e98 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -15,7 +15,6 @@ import org.opensearch.action.ActionModule; import org.opensearch.action.ActionModule.DynamicActionRegistry; import org.opensearch.action.admin.cluster.state.ClusterStateResponse; -import org.opensearch.client.Client; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.ClusterSettingsResponse; import org.opensearch.cluster.metadata.IndexMetadata; @@ -171,7 +170,7 @@ public List> getExtensionSettings() { Collections.emptyList() ); client = new NoOpNodeClient(this.getTestName()); - identityService = new IdentityService(Settings.EMPTY, mock(Client.class), List.of()); + identityService = new IdentityService(Settings.EMPTY, client, List.of()); } @Override From c172c55559cef2d0530bfd44834bf97f61c07ecb Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 11:34:16 -0500 Subject: [PATCH 10/17] Add runAs to client interface Signed-off-by: Craig Perkins --- .../identity/shiro/ShiroIdentityPlugin.java | 8 ++- .../identity/shiro/ShiroPluginSubject.java | 49 +++++++++++++++++++ .../shiro/ShiroIdentityPluginTests.java | 8 +-- .../java/org/opensearch/client/Client.java | 3 ++ .../client/support/AbstractClient.java | 7 +++ .../opensearch/identity/IdentityService.java | 12 ++--- .../opensearch/identity/PluginSubject.java | 19 +++++++ ...temClient.java => RunAsSubjectClient.java} | 18 ++++--- .../java/org/opensearch/identity/Subject.java | 8 +++ .../identity/noop/NoopIdentityPlugin.java | 14 +++--- .../identity/noop/NoopPluginSubject.java | 49 +++++++++++++++++++ .../main/java/org/opensearch/node/Node.java | 6 +-- .../plugins/IdentityAwarePlugin.java | 6 +-- .../opensearch/plugins/IdentityPlugin.java | 4 +- .../opensearch/action/ActionModuleTests.java | 5 +- .../bootstrap/IdentityPluginTests.java | 15 +++--- .../extensions/ExtensionsManagerTests.java | 2 +- .../rest/ExtensionRestRequestTests.java | 4 +- .../RestInitializeExtensionActionTests.java | 10 ++-- .../rest/RestSendToExtensionActionTests.java | 7 ++- ...ests.java => RunAsSubjectClientTests.java} | 27 ++++++---- 21 files changed, 208 insertions(+), 73 deletions(-) create mode 100644 plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroPluginSubject.java create mode 100644 server/src/main/java/org/opensearch/identity/PluginSubject.java rename server/src/main/java/org/opensearch/identity/{RunAsSystemClient.java => RunAsSubjectClient.java} (73%) create mode 100644 server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java rename server/src/test/java/org/opensearch/identity/{RunAsSystemClientTests.java => RunAsSubjectClientTests.java} (63%) diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java index 4167c0efb6ff7..2da788242a745 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java @@ -23,7 +23,7 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; import org.opensearch.env.NodeEnvironment; -import org.opensearch.identity.RunAsSystemClient; +import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.AuthToken; import org.opensearch.identity.tokens.TokenManager; @@ -54,7 +54,6 @@ public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin, private final ShiroTokenManager authTokenHandler; private ThreadPool threadPool; - private Client client; /** * Create a new instance of the Shiro Identity Plugin @@ -84,7 +83,6 @@ public Collection createComponents( Supplier repositoriesServiceSupplier ) { this.threadPool = threadPool; - this.client = client; return Collections.emptyList(); } @@ -140,7 +138,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } } - public Client getRunAsClient(Plugin plugin) { - return new RunAsSystemClient(client); + public PluginSubject getPluginSubject(Plugin plugin) { + return new ShiroPluginSubject(threadPool); } } diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroPluginSubject.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroPluginSubject.java new file mode 100644 index 0000000000000..31dde34f447d4 --- /dev/null +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroPluginSubject.java @@ -0,0 +1,49 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.identity.shiro; + +import org.opensearch.common.annotation.ExperimentalApi; +import org.opensearch.common.util.concurrent.ThreadContext; +import org.opensearch.identity.NamedPrincipal; +import org.opensearch.identity.PluginSubject; +import org.opensearch.threadpool.ThreadPool; + +import java.security.Principal; +import java.util.concurrent.Callable; + +/** + * Implementation of subject that is always authenticated + *

+ * This class and related classes in this package will not return nulls or fail permissions checks + * + * This class is used by the ShiroIdentityPlugin to initialize IdentityAwarePlugins + * + * @opensearch.experimental + */ +@ExperimentalApi +public class ShiroPluginSubject implements PluginSubject { + private final ThreadPool threadPool; + + ShiroPluginSubject(ThreadPool threadPool) { + super(); + this.threadPool = threadPool; + } + + @Override + public Principal getPrincipal() { + return NamedPrincipal.UNAUTHENTICATED; + } + + @Override + public T runAs(Callable callable) throws Exception { + try (ThreadContext.StoredContext ctx = threadPool.getThreadContext().stashContext()) { + return callable.call(); + } + } +} diff --git a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java index 2b40fda145aa7..9a09eb7a2a8c6 100644 --- a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java +++ b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java @@ -9,7 +9,6 @@ package org.opensearch.identity.shiro; import org.opensearch.OpenSearchException; -import org.opensearch.client.Client; import org.opensearch.common.settings.Settings; import org.opensearch.identity.IdentityService; import org.opensearch.plugins.IdentityPlugin; @@ -28,7 +27,7 @@ public class ShiroIdentityPluginTests extends OpenSearchTestCase { public void testSingleIdentityPluginSucceeds() { IdentityPlugin identityPlugin1 = new ShiroIdentityPlugin(Settings.EMPTY); List pluginList1 = List.of(identityPlugin1); - IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(Client.class), pluginList1); + IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList1); assertThat(identityService1.getTokenManager(), is(instanceOf(ShiroTokenManager.class))); } @@ -37,7 +36,10 @@ public void testMultipleIdentityPluginsFail() { IdentityPlugin identityPlugin2 = new ShiroIdentityPlugin(Settings.EMPTY); IdentityPlugin identityPlugin3 = new ShiroIdentityPlugin(Settings.EMPTY); List pluginList = List.of(identityPlugin1, identityPlugin2, identityPlugin3); - Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, mock(Client.class), pluginList)); + Exception ex = assertThrows( + OpenSearchException.class, + () -> new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList) + ); assert (ex.getMessage().contains("Multiple identity plugins are not supported,")); } diff --git a/server/src/main/java/org/opensearch/client/Client.java b/server/src/main/java/org/opensearch/client/Client.java index 322b435bdf35c..5ac3e54580037 100644 --- a/server/src/main/java/org/opensearch/client/Client.java +++ b/server/src/main/java/org/opensearch/client/Client.java @@ -91,6 +91,7 @@ import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; import org.opensearch.core.action.ActionListener; +import org.opensearch.identity.Subject; import java.util.Map; @@ -125,6 +126,8 @@ public interface Client extends OpenSearchClient, Releasable { */ AdminClient admin(); + Client runAs(Subject subject); + /** * Index a JSON source associated with a given index. *

diff --git a/server/src/main/java/org/opensearch/client/support/AbstractClient.java b/server/src/main/java/org/opensearch/client/support/AbstractClient.java index f4683ab516cef..b7ac801aabcfa 100644 --- a/server/src/main/java/org/opensearch/client/support/AbstractClient.java +++ b/server/src/main/java/org/opensearch/client/support/AbstractClient.java @@ -425,6 +425,8 @@ import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.tasks.TaskId; import org.opensearch.core.xcontent.MediaType; +import org.opensearch.identity.RunAsSubjectClient; +import org.opensearch.identity.Subject; import org.opensearch.threadpool.ThreadPool; import java.util.Map; @@ -464,6 +466,11 @@ public final AdminClient admin() { return admin; } + @Override + public final Client runAs(Subject subject) { + return new RunAsSubjectClient(this, subject); + } + @Override public final ActionFuture execute( ActionType action, diff --git a/server/src/main/java/org/opensearch/identity/IdentityService.java b/server/src/main/java/org/opensearch/identity/IdentityService.java index bbe2aaaa2eeca..33066fae5a80d 100644 --- a/server/src/main/java/org/opensearch/identity/IdentityService.java +++ b/server/src/main/java/org/opensearch/identity/IdentityService.java @@ -8,7 +8,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.OpenSearchException; -import org.opensearch.client.Client; import org.opensearch.common.annotation.InternalApi; import org.opensearch.common.settings.Settings; import org.opensearch.identity.noop.NoopIdentityPlugin; @@ -16,6 +15,7 @@ import org.opensearch.plugins.IdentityAwarePlugin; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.plugins.Plugin; +import org.opensearch.threadpool.ThreadPool; import java.util.List; import java.util.stream.Collectors; @@ -30,16 +30,14 @@ public class IdentityService { private static final Logger log = LogManager.getLogger(IdentityService.class); private final Settings settings; - private final Client client; private final IdentityPlugin identityPlugin; - public IdentityService(final Settings settings, final Client client, final List identityPlugins) { + public IdentityService(final Settings settings, final ThreadPool threadPool, final List identityPlugins) { this.settings = settings; - this.client = client; if (identityPlugins.size() == 0) { log.debug("Identity plugins size is 0"); - identityPlugin = new NoopIdentityPlugin(client); + identityPlugin = new NoopIdentityPlugin(threadPool); } else if (identityPlugins.size() == 1) { log.debug("Identity plugins size is 1"); identityPlugin = identityPlugins.get(0); @@ -68,8 +66,8 @@ public TokenManager getTokenManager() { public void initializeIdentityAwarePlugins(final List identityAwarePlugins) { if (identityAwarePlugins != null) { for (IdentityAwarePlugin plugin : identityAwarePlugins) { - Client client = identityPlugin.getRunAsClient((Plugin) plugin); - plugin.assignRunAsClient(client); + PluginSubject pluginSubject = identityPlugin.getPluginSubject((Plugin) plugin); + plugin.assignSubject(pluginSubject); } } } diff --git a/server/src/main/java/org/opensearch/identity/PluginSubject.java b/server/src/main/java/org/opensearch/identity/PluginSubject.java new file mode 100644 index 0000000000000..3ea42182d3fc3 --- /dev/null +++ b/server/src/main/java/org/opensearch/identity/PluginSubject.java @@ -0,0 +1,19 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.identity; + +import org.opensearch.common.annotation.ExperimentalApi; + +/** + * Similar to {@link Subject}, but represents a plugin executing actions + * + * @opensearch.experimental + */ +@ExperimentalApi +public interface PluginSubject extends Subject {} diff --git a/server/src/main/java/org/opensearch/identity/RunAsSystemClient.java b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java similarity index 73% rename from server/src/main/java/org/opensearch/identity/RunAsSystemClient.java rename to server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java index da7b9a2d98a69..acc6e1dbf994d 100644 --- a/server/src/main/java/org/opensearch/identity/RunAsSystemClient.java +++ b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java @@ -18,18 +18,21 @@ import org.opensearch.core.action.ActionResponse; /** - * Implementation of client that will run transport actions in a stashed context - *

- * This class and related classes in this package will not return nulls or fail permissions checks - * - * This class is used by the NoopIdentityPlugin to initialize IdentityAwarePlugins + * Implementation of client that will run transport actions in a stashed context and inject the name of the provided + * subject into the context. * * @opensearch.internal */ @InternalApi -public class RunAsSystemClient extends FilterClient { - public RunAsSystemClient(Client delegate) { +public class RunAsSubjectClient extends FilterClient { + + public static final String SUBJECT_TRANSIENT_NAME = "subject.name"; + + private final Subject subject; + + public RunAsSubjectClient(Client delegate, Subject subject) { super(delegate); + this.subject = subject; } @Override @@ -40,6 +43,7 @@ protected void ) { ThreadContext threadContext = threadPool().getThreadContext(); try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { + threadContext.putTransient(SUBJECT_TRANSIENT_NAME, subject.getPrincipal().getName()); super.doExecute(action, request, ActionListener.runBefore(listener, ctx::restore)); } } diff --git a/server/src/main/java/org/opensearch/identity/Subject.java b/server/src/main/java/org/opensearch/identity/Subject.java index 75bc3460c1be4..0fb0e53848d80 100644 --- a/server/src/main/java/org/opensearch/identity/Subject.java +++ b/server/src/main/java/org/opensearch/identity/Subject.java @@ -8,6 +8,7 @@ import org.opensearch.common.annotation.ExperimentalApi; import java.security.Principal; +import java.util.concurrent.Callable; /** * An individual, process, or device that causes information to flow among objects or change to the system state. @@ -21,4 +22,11 @@ public interface Subject { * Get the application-wide uniquely identifying principal * */ Principal getPrincipal(); + + /** + * runAs allows the caller to run a callable function as this subject + */ + default T runAs(Callable callable) throws Exception { + return callable.call(); + }; } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java b/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java index e08b1d2de7f49..6279388c76f96 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java @@ -8,12 +8,12 @@ package org.opensearch.identity.noop; -import org.opensearch.client.Client; -import org.opensearch.identity.RunAsSystemClient; +import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.plugins.Plugin; +import org.opensearch.threadpool.ThreadPool; /** * Implementation of identity plugin that does not enforce authentication or authorization @@ -24,10 +24,10 @@ */ public class NoopIdentityPlugin implements IdentityPlugin { - private final Client client; + private final ThreadPool threadPool; - public NoopIdentityPlugin(Client client) { - this.client = client; + public NoopIdentityPlugin(ThreadPool threadPool) { + this.threadPool = threadPool; } /** @@ -49,7 +49,7 @@ public TokenManager getTokenManager() { } @Override - public Client getRunAsClient(Plugin plugin) { - return new RunAsSystemClient(client); + public PluginSubject getPluginSubject(Plugin plugin) { + return new NoopPluginSubject(threadPool); } } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java b/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java new file mode 100644 index 0000000000000..c14090a90b10e --- /dev/null +++ b/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java @@ -0,0 +1,49 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.identity.noop; + +import org.opensearch.common.annotation.InternalApi; +import org.opensearch.common.util.concurrent.ThreadContext; +import org.opensearch.identity.NamedPrincipal; +import org.opensearch.identity.PluginSubject; +import org.opensearch.threadpool.ThreadPool; + +import java.security.Principal; +import java.util.concurrent.Callable; + +/** + * Implementation of subject that is always authenticated + *

+ * This class and related classes in this package will not return nulls or fail permissions checks + * + * This class is used by the NoopIdentityPlugin to initialize IdentityAwarePlugins + * + * @opensearch.internal + */ +@InternalApi +public class NoopPluginSubject implements PluginSubject { + private final ThreadPool threadPool; + + public NoopPluginSubject(ThreadPool threadPool) { + super(); + this.threadPool = threadPool; + } + + @Override + public Principal getPrincipal() { + return NamedPrincipal.UNAUTHENTICATED; + } + + @Override + public T runAs(Callable callable) throws Exception { + try (ThreadContext.StoredContext ctx = threadPool.getThreadContext().stashContext()) { + return callable.call(); + } + } +} diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index eb8c47b2eb945..dc4ebc2f9338d 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -588,9 +588,7 @@ protected Node( runnableTaskListener = new AtomicReference<>(); final ThreadPool threadPool = new ThreadPool(settings, runnableTaskListener, executorBuilders.toArray(new ExecutorBuilder[0])); - client = new NodeClient(settings, threadPool); - - final IdentityService identityService = new IdentityService(settings, client, identityPlugins); + final IdentityService identityService = new IdentityService(settings, threadPool, identityPlugins); if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { final List extensionAwarePlugins = pluginsService.filterPlugins(ExtensionAwarePlugin.class); @@ -625,6 +623,8 @@ protected Node( additionalSettings.addAll(builder.getRegisteredSettings()); } + client = new NodeClient(settings, threadPool); + final ScriptModule scriptModule = new ScriptModule(settings, pluginsService.filterPlugins(ScriptPlugin.class)); final ScriptService scriptService = newScriptService(settings, scriptModule.engines, scriptModule.contexts); AnalysisModule analysisModule = new AnalysisModule(this.environment, pluginsService.filterPlugins(AnalysisPlugin.class)); diff --git a/server/src/main/java/org/opensearch/plugins/IdentityAwarePlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityAwarePlugin.java index 9acc162c1fdf8..b19dcfe5544ec 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityAwarePlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityAwarePlugin.java @@ -8,8 +8,8 @@ package org.opensearch.plugins; -import org.opensearch.client.Client; import org.opensearch.common.annotation.ExperimentalApi; +import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; /** @@ -27,8 +27,8 @@ public interface IdentityAwarePlugin { /** * Passes necessary classes for this plugin to operate as an IdentityAwarePlugin * - * @param pluginClient A subject for running transport actions in the plugin context for system index + * @param pluginSubject A subject for running transport actions in the plugin context for system index * interaction */ - default void assignRunAsClient(Client pluginClient) {} + default void assignSubject(PluginSubject pluginSubject) {} } diff --git a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java index 70aab42f62651..b40af14231fb9 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java @@ -8,8 +8,8 @@ package org.opensearch.plugins; -import org.opensearch.client.Client; import org.opensearch.common.annotation.ExperimentalApi; +import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; @@ -41,5 +41,5 @@ public interface IdentityPlugin { * @param plugin The corresponding plugin * @return Subject corresponding to the plugin */ - Client getRunAsClient(Plugin plugin); + PluginSubject getPluginSubject(Plugin plugin); } diff --git a/server/src/test/java/org/opensearch/action/ActionModuleTests.java b/server/src/test/java/org/opensearch/action/ActionModuleTests.java index d2d37650881e7..6b8951dd43d11 100644 --- a/server/src/test/java/org/opensearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/opensearch/action/ActionModuleTests.java @@ -36,7 +36,6 @@ import org.opensearch.action.main.TransportMainAction; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.TransportAction; -import org.opensearch.client.Client; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.node.DiscoveryNodes; @@ -144,8 +143,8 @@ public void testSetupRestHandlerContainsKnownBuiltin() throws IOException { null, usageService, null, - new IdentityService(Settings.EMPTY, mock(Client.class), new ArrayList<>()), - new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(Client.class), List.of())) + new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()), + new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of())) ); actionModule.initRestHandlers(null); // At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail diff --git a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java index af3486fd5ab01..e9877f66af05f 100644 --- a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java +++ b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java @@ -23,17 +23,15 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; public class IdentityPluginTests extends OpenSearchTestCase { public void testSingleIdentityPluginSucceeds() { Client client = mock(Client.class); TestThreadPool threadPool = new TestThreadPool(getTestName()); - when(client.threadPool()).thenReturn(threadPool); - IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(client); + IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(threadPool); List pluginList1 = List.of(identityPlugin1); - IdentityService identityService1 = new IdentityService(Settings.EMPTY, client, pluginList1); + IdentityService identityService1 = new IdentityService(Settings.EMPTY, threadPool, pluginList1); assertTrue(identityService1.getCurrentSubject().getPrincipal().getName().equalsIgnoreCase("Unauthenticated")); assertThat(identityService1.getTokenManager(), is(instanceOf(NoopTokenManager.class))); terminate(threadPool); @@ -42,12 +40,11 @@ public void testSingleIdentityPluginSucceeds() { public void testMultipleIdentityPluginsFail() { Client client = mock(Client.class); TestThreadPool threadPool = new TestThreadPool(getTestName()); - when(client.threadPool()).thenReturn(threadPool); - IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(client); - IdentityPlugin identityPlugin2 = new NoopIdentityPlugin(client); - IdentityPlugin identityPlugin3 = new NoopIdentityPlugin(client); + IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(threadPool); + IdentityPlugin identityPlugin2 = new NoopIdentityPlugin(threadPool); + IdentityPlugin identityPlugin3 = new NoopIdentityPlugin(threadPool); List pluginList = List.of(identityPlugin1, identityPlugin2, identityPlugin3); - Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, client, pluginList)); + Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, threadPool, pluginList)); assert (ex.getMessage().contains("Multiple identity plugins are not supported,")); terminate(threadPool); } diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 2d30ba8791e98..bf1d52b49cb1f 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -170,7 +170,7 @@ public List> getExtensionSettings() { Collections.emptyList() ); client = new NoOpNodeClient(this.getTestName()); - identityService = new IdentityService(Settings.EMPTY, client, List.of()); + identityService = new IdentityService(Settings.EMPTY, threadPool, List.of()); } @Override diff --git a/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java b/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java index 677486b769694..7d9ebe1d1e26a 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java @@ -9,7 +9,6 @@ package org.opensearch.extensions.rest; import org.opensearch.OpenSearchParseException; -import org.opensearch.client.Client; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.settings.Settings; import org.opensearch.core.common.bytes.BytesArray; @@ -30,6 +29,7 @@ import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestRequest.Method; import org.opensearch.test.OpenSearchTestCase; +import org.opensearch.threadpool.ThreadPool; import java.nio.charset.StandardCharsets; import java.security.Principal; @@ -74,7 +74,7 @@ public void setUp() throws Exception { userPrincipal = () -> "user1"; expectedHttpVersion = HttpRequest.HttpVersion.HTTP_1_1; extensionTokenProcessor = "placeholder_extension_token_processor"; - identityService = new IdentityService(Settings.EMPTY, mock(Client.class), List.of()); + identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()); TokenManager tokenManager = identityService.getTokenManager(); Subject subject = this.identityService.getCurrentSubject(); OnBehalfOfClaims claims = new OnBehalfOfClaims("testID", subject.getPrincipal().getName()); diff --git a/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java b/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java index a725017de6b2a..ac818c3bb4a7b 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java @@ -9,7 +9,6 @@ package org.opensearch.extensions.rest; import org.opensearch.Version; -import org.opensearch.client.Client; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.common.network.NetworkService; import org.opensearch.common.settings.Setting; @@ -122,10 +121,7 @@ public void testRestInitializeExtensionActionResponse() throws Exception { } public void testRestInitializeExtensionActionFailure() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager( - Set.of(), - new IdentityService(Settings.EMPTY, mock(Client.class), List.of()) - ); + ExtensionsManager extensionsManager = new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, threadPool, List.of())); RestInitializeExtensionAction restInitializeExtensionAction = new RestInitializeExtensionAction(extensionsManager); final String content = "{\"name\":\"ad-extension\",\"uniqueId\":\"\",\"hostAddress\":\"127.0.0.1\"," @@ -160,7 +156,7 @@ public void testRestInitializeExtensionActionResponseWithAdditionalSettings() th ); ExtensionsManager extensionsManager = new ExtensionsManager( Set.of(boolSetting, stringSetting, intSetting, listSetting), - new IdentityService(Settings.EMPTY, mock(Client.class), List.of()) + new IdentityService(Settings.EMPTY, threadPool, List.of()) ); ExtensionsManager spy = spy(extensionsManager); @@ -210,7 +206,7 @@ public void testRestInitializeExtensionActionResponseWithAdditionalSettingsUsing ); ExtensionsManager extensionsManager = new ExtensionsManager( Set.of(boolSetting, stringSetting, intSetting, listSetting), - new IdentityService(Settings.EMPTY, mock(Client.class), List.of()) + new IdentityService(Settings.EMPTY, threadPool, List.of()) ); ExtensionsManager spy = spy(extensionsManager); diff --git a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java index a0a3bdcf3b818..e9c910ea361fb 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java @@ -14,7 +14,6 @@ import org.opensearch.action.admin.cluster.health.ClusterHealthAction; import org.opensearch.action.admin.cluster.health.TransportClusterHealthAction; import org.opensearch.action.support.ActionFilters; -import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.common.network.NetworkService; @@ -123,10 +122,10 @@ public void setup() throws Exception { null, usageService, null, - new IdentityService(Settings.EMPTY, mock(Client.class), new ArrayList<>()), - new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(Client.class), List.of())) + new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()), + new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of())) ); - identityService = new IdentityService(Settings.EMPTY, mock(Client.class), new ArrayList<>()); + identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()); dynamicActionRegistry = actionModule.getDynamicActionRegistry(); } diff --git a/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java b/server/src/test/java/org/opensearch/identity/RunAsSubjectClientTests.java similarity index 63% rename from server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java rename to server/src/test/java/org/opensearch/identity/RunAsSubjectClientTests.java index 2a7ce36cac3a3..775420a8ea2ff 100644 --- a/server/src/test/java/org/opensearch/identity/RunAsSystemClientTests.java +++ b/server/src/test/java/org/opensearch/identity/RunAsSubjectClientTests.java @@ -10,27 +10,35 @@ import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; -import org.opensearch.client.Client; import org.opensearch.core.action.ActionListener; import org.opensearch.test.OpenSearchSingleNodeTestCase; import org.junit.Before; +import java.security.Principal; + import static org.hamcrest.Matchers.equalTo; -public class RunAsSystemClientTests extends OpenSearchSingleNodeTestCase { +public class RunAsSubjectClientTests extends OpenSearchSingleNodeTestCase { + + private final Subject TEST_SUBJECT = new Subject() { + @Override + public Principal getPrincipal() { + return new NamedPrincipal("testSubject"); + } + }; + @Before public void setup() { client().threadPool().getThreadContext().stashContext(); // start with fresh context } public void testThatContextIsRestoredOnActionListenerResponse() throws Exception { - Client systemClient = new RunAsSystemClient(client()); - systemClient.threadPool().getThreadContext().putHeader("test_header", "foo"); + client().threadPool().getThreadContext().putHeader("test_header", "foo"); - systemClient.admin().cluster().health(new ClusterHealthRequest(), new ActionListener<>() { + client().runAs(TEST_SUBJECT).admin().cluster().health(new ClusterHealthRequest(), new ActionListener<>() { @Override public void onResponse(ClusterHealthResponse clusterHealthResponse) { - String testHeader = systemClient.threadPool().getThreadContext().getHeader("test_header"); + String testHeader = client().threadPool().getThreadContext().getHeader("test_header"); assertThat(testHeader, equalTo("foo")); } @@ -42,10 +50,9 @@ public void onFailure(Exception e) { } public void testThatContextIsRestoredOnActionListenerFailure() throws Exception { - Client systemClient = new RunAsSystemClient(client()); - systemClient.threadPool().getThreadContext().putHeader("test_header", "bar"); + client().threadPool().getThreadContext().putHeader("test_header", "bar"); - systemClient.admin().cluster().health(new ClusterHealthRequest("dne"), new ActionListener<>() { + client().runAs(TEST_SUBJECT).admin().cluster().health(new ClusterHealthRequest("dne"), new ActionListener<>() { @Override public void onResponse(ClusterHealthResponse clusterHealthResponse) { fail("Expected cluster health action to fail"); @@ -53,7 +60,7 @@ public void onResponse(ClusterHealthResponse clusterHealthResponse) { @Override public void onFailure(Exception e) { - String testHeader = systemClient.threadPool().getThreadContext().getHeader("test_header"); + String testHeader = client().threadPool().getThreadContext().getHeader("test_header"); assertThat(testHeader, equalTo("bar")); } }); From b53e726b5aeaf350fedf5cae9ec1e2b9308bfc24 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 11:35:26 -0500 Subject: [PATCH 11/17] Cleanup Signed-off-by: Craig Perkins --- .../org/opensearch/identity/shiro/ShiroIdentityPluginTests.java | 1 + .../java/org/opensearch/identity/noop/NoopPluginSubject.java | 2 +- server/src/main/java/org/opensearch/node/Node.java | 1 - 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java index 9a09eb7a2a8c6..a15538e48bd66 100644 --- a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java +++ b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java @@ -13,6 +13,7 @@ import org.opensearch.identity.IdentityService; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.test.OpenSearchTestCase; +import org.opensearch.threadpool.ThreadPool; import java.util.List; diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java b/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java index c14090a90b10e..20e075276f317 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopPluginSubject.java @@ -30,7 +30,7 @@ public class NoopPluginSubject implements PluginSubject { private final ThreadPool threadPool; - public NoopPluginSubject(ThreadPool threadPool) { + NoopPluginSubject(ThreadPool threadPool) { super(); this.threadPool = threadPool; } diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index dc4ebc2f9338d..704a23890b07a 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -622,7 +622,6 @@ protected Node( for (final ExecutorBuilder builder : threadPool.builders()) { additionalSettings.addAll(builder.getRegisteredSettings()); } - client = new NodeClient(settings, threadPool); final ScriptModule scriptModule = new ScriptModule(settings, pluginsService.filterPlugins(ScriptPlugin.class)); From 5415ab3447dd502a20fa50874f755151b5b092a5 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 11:37:38 -0500 Subject: [PATCH 12/17] More cleanup Signed-off-by: Craig Perkins --- .../bootstrap/IdentityPluginTests.java | 4 -- .../identity/noop/NoopPluginSubjectTests.java | 58 +++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 server/src/test/java/org/opensearch/identity/noop/NoopPluginSubjectTests.java diff --git a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java index e9877f66af05f..d7b9f5917c366 100644 --- a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java +++ b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java @@ -9,7 +9,6 @@ package org.opensearch.bootstrap; import org.opensearch.OpenSearchException; -import org.opensearch.client.Client; import org.opensearch.common.settings.Settings; import org.opensearch.identity.IdentityService; import org.opensearch.identity.noop.NoopIdentityPlugin; @@ -22,12 +21,10 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; -import static org.mockito.Mockito.mock; public class IdentityPluginTests extends OpenSearchTestCase { public void testSingleIdentityPluginSucceeds() { - Client client = mock(Client.class); TestThreadPool threadPool = new TestThreadPool(getTestName()); IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(threadPool); List pluginList1 = List.of(identityPlugin1); @@ -38,7 +35,6 @@ public void testSingleIdentityPluginSucceeds() { } public void testMultipleIdentityPluginsFail() { - Client client = mock(Client.class); TestThreadPool threadPool = new TestThreadPool(getTestName()); IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(threadPool); IdentityPlugin identityPlugin2 = new NoopIdentityPlugin(threadPool); diff --git a/server/src/test/java/org/opensearch/identity/noop/NoopPluginSubjectTests.java b/server/src/test/java/org/opensearch/identity/noop/NoopPluginSubjectTests.java new file mode 100644 index 0000000000000..79c26a7eb790d --- /dev/null +++ b/server/src/test/java/org/opensearch/identity/noop/NoopPluginSubjectTests.java @@ -0,0 +1,58 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.identity.noop; + +import org.opensearch.common.settings.Settings; +import org.opensearch.identity.IdentityService; +import org.opensearch.identity.NamedPrincipal; +import org.opensearch.identity.PluginSubject; +import org.opensearch.plugins.IdentityAwarePlugin; +import org.opensearch.plugins.Plugin; +import org.opensearch.test.OpenSearchTestCase; +import org.opensearch.threadpool.TestThreadPool; +import org.opensearch.threadpool.ThreadPool; + +import java.util.List; + +import static org.hamcrest.Matchers.equalTo; + +public class NoopPluginSubjectTests extends OpenSearchTestCase { + public static class TestPlugin extends Plugin implements IdentityAwarePlugin { + private PluginSubject subject; + + @Override + public void assignSubject(PluginSubject subject) { + this.subject = subject; + } + + public PluginSubject getSubject() { + return subject; + } + } + + public void testInitializeIdentityAwarePlugin() throws Exception { + ThreadPool threadPool = new TestThreadPool(getTestName()); + IdentityService identityService = new IdentityService(Settings.EMPTY, threadPool, List.of()); + + TestPlugin testPlugin = new TestPlugin(); + identityService.initializeIdentityAwarePlugins(List.of(testPlugin)); + + PluginSubject testPluginSubject = new NoopPluginSubject(threadPool); + assertThat(testPlugin.getSubject().getPrincipal().getName(), equalTo(NamedPrincipal.UNAUTHENTICATED.getName())); + assertThat(testPluginSubject.getPrincipal().getName(), equalTo(NamedPrincipal.UNAUTHENTICATED.getName())); + threadPool.getThreadContext().putHeader("test_header", "foo"); + assertThat(threadPool.getThreadContext().getHeader("test_header"), equalTo("foo")); + testPluginSubject.runAs(() -> { + assertNull(threadPool.getThreadContext().getHeader("test_header")); + return null; + }); + assertThat(threadPool.getThreadContext().getHeader("test_header"), equalTo("foo")); + terminate(threadPool); + } +} From 95f74c958a0d4e1d7711db4463425804c58dc288 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 14:26:37 -0500 Subject: [PATCH 13/17] Add tests to verify that subject is injected Signed-off-by: Craig Perkins --- CHANGELOG.md | 2 +- .../identity/RunAsSubjectClient.java | 5 ++ .../identity/RunAsSubjectClientTests.java | 84 ++++++++++++------- 3 files changed, 62 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b073b51968c5c..cea3d9406f75f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,7 +71,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Allow extended plugins to be optional ([#16909](https://github.com/opensearch-project/OpenSearch/pull/16909)) - Use the correct type to widen the sort fields when merging top docs ([#16881](https://github.com/opensearch-project/OpenSearch/pull/16881)) - Limit reader writer separation to remote store enabled clusters [#16760](https://github.com/opensearch-project/OpenSearch/pull/16760) -- Refactor IdentityAwarePlugin interface to be assigned a client for executing actions ([#16976](https://github.com/opensearch-project/OpenSearch/pull/16976)) +- Add runAs(Subject subject) to Client interface ([#16976](https://github.com/opensearch-project/OpenSearch/pull/16976)) ### Deprecated - Performing update operation with default pipeline or final pipeline is deprecated ([#16712](https://github.com/opensearch-project/OpenSearch/pull/16712)) diff --git a/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java index acc6e1dbf994d..82fc2165a03cd 100644 --- a/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java +++ b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java @@ -8,6 +8,8 @@ package org.opensearch.identity; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.opensearch.action.ActionRequest; import org.opensearch.action.ActionType; import org.opensearch.client.Client; @@ -26,6 +28,8 @@ @InternalApi public class RunAsSubjectClient extends FilterClient { + private static final Logger logger = LogManager.getLogger(RunAsSubjectClient.class); + public static final String SUBJECT_TRANSIENT_NAME = "subject.name"; private final Subject subject; @@ -44,6 +48,7 @@ protected void ThreadContext threadContext = threadPool().getThreadContext(); try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { threadContext.putTransient(SUBJECT_TRANSIENT_NAME, subject.getPrincipal().getName()); + logger.info("Running transport action with subject: {}", subject.getPrincipal().getName()); super.doExecute(action, request, ActionListener.runBefore(listener, ctx::restore)); } } diff --git a/server/src/test/java/org/opensearch/identity/RunAsSubjectClientTests.java b/server/src/test/java/org/opensearch/identity/RunAsSubjectClientTests.java index 775420a8ea2ff..e85baa6488396 100644 --- a/server/src/test/java/org/opensearch/identity/RunAsSubjectClientTests.java +++ b/server/src/test/java/org/opensearch/identity/RunAsSubjectClientTests.java @@ -8,9 +8,12 @@ package org.opensearch.identity; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; import org.opensearch.core.action.ActionListener; +import org.opensearch.test.MockLogAppender; import org.opensearch.test.OpenSearchSingleNodeTestCase; import org.junit.Before; @@ -33,36 +36,61 @@ public void setup() { } public void testThatContextIsRestoredOnActionListenerResponse() throws Exception { - client().threadPool().getThreadContext().putHeader("test_header", "foo"); - - client().runAs(TEST_SUBJECT).admin().cluster().health(new ClusterHealthRequest(), new ActionListener<>() { - @Override - public void onResponse(ClusterHealthResponse clusterHealthResponse) { - String testHeader = client().threadPool().getThreadContext().getHeader("test_header"); - assertThat(testHeader, equalTo("foo")); - } - - @Override - public void onFailure(Exception e) { - fail("Expected cluster health action to succeed"); - } - }); + try (MockLogAppender mockLogAppender = MockLogAppender.createForLoggers(LogManager.getLogger(RunAsSubjectClient.class))) { + mockLogAppender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "testSubject", + "org.opensearch.identity.RunAsSubjectClient", + Level.INFO, + "Running transport action with subject: testSubject" + ) + ); + + client().threadPool().getThreadContext().putHeader("test_header", "foo"); + + client().runAs(TEST_SUBJECT).admin().cluster().health(new ClusterHealthRequest(), new ActionListener<>() { + @Override + public void onResponse(ClusterHealthResponse clusterHealthResponse) { + String testHeader = client().threadPool().getThreadContext().getHeader("test_header"); + assertThat(testHeader, equalTo("foo")); + + mockLogAppender.assertAllExpectationsMatched(); + } + + @Override + public void onFailure(Exception e) { + fail("Expected cluster health action to succeed"); + } + }); + } } public void testThatContextIsRestoredOnActionListenerFailure() throws Exception { - client().threadPool().getThreadContext().putHeader("test_header", "bar"); - - client().runAs(TEST_SUBJECT).admin().cluster().health(new ClusterHealthRequest("dne"), new ActionListener<>() { - @Override - public void onResponse(ClusterHealthResponse clusterHealthResponse) { - fail("Expected cluster health action to fail"); - } - - @Override - public void onFailure(Exception e) { - String testHeader = client().threadPool().getThreadContext().getHeader("test_header"); - assertThat(testHeader, equalTo("bar")); - } - }); + try (MockLogAppender mockLogAppender = MockLogAppender.createForLoggers(LogManager.getLogger(RunAsSubjectClient.class))) { + mockLogAppender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "testSubject", + "org.opensearch.identity.RunAsSubjectClient", + Level.INFO, + "Running transport action with subject: testSubject" + ) + ); + client().threadPool().getThreadContext().putHeader("test_header", "bar"); + + client().runAs(TEST_SUBJECT).admin().cluster().health(new ClusterHealthRequest("dne"), new ActionListener<>() { + @Override + public void onResponse(ClusterHealthResponse clusterHealthResponse) { + fail("Expected cluster health action to fail"); + } + + @Override + public void onFailure(Exception e) { + String testHeader = client().threadPool().getThreadContext().getHeader("test_header"); + assertThat(testHeader, equalTo("bar")); + + mockLogAppender.assertAllExpectationsMatched(); + } + }); + } } } From 21bc3d9ada230a038a8ce947f95f270ba3dba0bc Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 15:15:32 -0500 Subject: [PATCH 14/17] Create constants file for identity constants Signed-off-by: Craig Perkins --- .../opensearch/identity/IdentityConstants.java | 18 ++++++++++++++++++ .../identity/RunAsSubjectClient.java | 4 ++-- 2 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 server/src/main/java/org/opensearch/identity/IdentityConstants.java diff --git a/server/src/main/java/org/opensearch/identity/IdentityConstants.java b/server/src/main/java/org/opensearch/identity/IdentityConstants.java new file mode 100644 index 0000000000000..b9af6c8dea265 --- /dev/null +++ b/server/src/main/java/org/opensearch/identity/IdentityConstants.java @@ -0,0 +1,18 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.identity; + +/** + * Class containing constants used in the identity module + * + * @opensearch.experimental + */ +public class IdentityConstants { + public static final String SUBJECT_TRANSIENT_NAME = "subject.name"; +} diff --git a/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java index 82fc2165a03cd..dc3b9fdf93bba 100644 --- a/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java +++ b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java @@ -19,6 +19,8 @@ import org.opensearch.core.action.ActionListener; import org.opensearch.core.action.ActionResponse; +import static org.opensearch.identity.IdentityConstants.SUBJECT_TRANSIENT_NAME; + /** * Implementation of client that will run transport actions in a stashed context and inject the name of the provided * subject into the context. @@ -30,8 +32,6 @@ public class RunAsSubjectClient extends FilterClient { private static final Logger logger = LogManager.getLogger(RunAsSubjectClient.class); - public static final String SUBJECT_TRANSIENT_NAME = "subject.name"; - private final Subject subject; public RunAsSubjectClient(Client delegate, Subject subject) { From 0531f35485d5335b5183e009fcc9196a67d9eb53 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 15:45:03 -0500 Subject: [PATCH 15/17] Use subject for header, not only the name Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/identity/RunAsSubjectClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java index dc3b9fdf93bba..90517914401c8 100644 --- a/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java +++ b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java @@ -47,7 +47,7 @@ protected void ) { ThreadContext threadContext = threadPool().getThreadContext(); try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { - threadContext.putTransient(SUBJECT_TRANSIENT_NAME, subject.getPrincipal().getName()); + threadContext.putTransient(SUBJECT_TRANSIENT_NAME, subject); logger.info("Running transport action with subject: {}", subject.getPrincipal().getName()); super.doExecute(action, request, ActionListener.runBefore(listener, ctx::restore)); } From b30c7398cc87df837661834f391b1454d1c9e280 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 16:04:24 -0500 Subject: [PATCH 16/17] Surround with subject.runAs Signed-off-by: Craig Perkins --- .../opensearch/identity/RunAsSubjectClient.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java index 90517914401c8..8cef60a718604 100644 --- a/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java +++ b/server/src/main/java/org/opensearch/identity/RunAsSubjectClient.java @@ -19,8 +19,6 @@ import org.opensearch.core.action.ActionListener; import org.opensearch.core.action.ActionResponse; -import static org.opensearch.identity.IdentityConstants.SUBJECT_TRANSIENT_NAME; - /** * Implementation of client that will run transport actions in a stashed context and inject the name of the provided * subject into the context. @@ -45,11 +43,14 @@ protected void Request request, ActionListener listener ) { - ThreadContext threadContext = threadPool().getThreadContext(); - try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { - threadContext.putTransient(SUBJECT_TRANSIENT_NAME, subject); - logger.info("Running transport action with subject: {}", subject.getPrincipal().getName()); - super.doExecute(action, request, ActionListener.runBefore(listener, ctx::restore)); + try (ThreadContext.StoredContext ctx = threadPool().getThreadContext().newStoredContext(false)) { + subject.runAs(() -> { + logger.info("Running transport action with subject: {}", subject.getPrincipal().getName()); + super.doExecute(action, request, ActionListener.runBefore(listener, ctx::restore)); + return null; + }); + } catch (Exception e) { + throw new RuntimeException(e); } } } From 0728709eb646d755b92ac4aa96b1559e95a845ae Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 16:04:48 -0500 Subject: [PATCH 17/17] Remove IdentityConstants Signed-off-by: Craig Perkins --- .../opensearch/identity/IdentityConstants.java | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/identity/IdentityConstants.java diff --git a/server/src/main/java/org/opensearch/identity/IdentityConstants.java b/server/src/main/java/org/opensearch/identity/IdentityConstants.java deleted file mode 100644 index b9af6c8dea265..0000000000000 --- a/server/src/main/java/org/opensearch/identity/IdentityConstants.java +++ /dev/null @@ -1,18 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.identity; - -/** - * Class containing constants used in the identity module - * - * @opensearch.experimental - */ -public class IdentityConstants { - public static final String SUBJECT_TRANSIENT_NAME = "subject.name"; -}