From e6b82ba19e6c66a83709b5dd6e85b36f299b1146 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 26 Aug 2024 16:47:44 -0400 Subject: [PATCH 01/15] Add authenticate to IdentityPlugin interface Signed-off-by: Craig Perkins --- .../identity/shiro/DelegatingRestHandler.java | 80 +++++++++++ .../identity/shiro/ShiroIdentityPlugin.java | 44 +++++- .../identity/shiro/ShiroSecurityManager.java | 2 - .../identity/shiro/ShiroSubject.java | 2 - .../identity/shiro/ShiroTokenManager.java | 2 - .../shiro/realm/BCryptPasswordMatcher.java | 2 - .../identity/shiro/realm/OpenSearchRealm.java | 2 - .../opensearch/identity/shiro/realm/User.java | 2 - .../shiro/ShiroIdentityPluginTests.java | 9 +- .../org/opensearch/action/ActionModule.java | 28 ++-- .../extensions/NoopExtensionsManager.java | 5 +- .../opensearch/identity/IdentityService.java | 14 +- .../main/java/org/opensearch/node/Node.java | 26 ++-- .../opensearch/plugins/IdentityPlugin.java | 19 ++- .../org/opensearch/rest/RestController.java | 55 +------- .../opensearch/action/ActionModuleTests.java | 132 +++++++++++++++++- .../bootstrap/IdentityPluginTests.java | 9 +- .../extensions/ExtensionsManagerTests.java | 5 +- .../rest/ExtensionRestRequestTests.java | 4 +- .../RestInitializeExtensionActionTests.java | 6 +- .../rest/RestSendToExtensionActionTests.java | 6 +- .../opensearch/rest/RestControllerTests.java | 29 ++-- .../rest/RestHttpResponseHeadersTests.java | 12 +- .../indices/RestValidateQueryActionTests.java | 12 +- .../test/rest/RestActionTestCase.java | 12 +- 25 files changed, 355 insertions(+), 164 deletions(-) create mode 100644 plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java new file mode 100644 index 0000000000000..2108b42e258ab --- /dev/null +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java @@ -0,0 +1,80 @@ +/* + * 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.client.node.NodeClient; +import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestHandler; +import org.opensearch.rest.RestRequest; + +import java.util.List; +import java.util.Objects; + +/** + * Delegating RestHandler that delegates all implementations to original handler + */ +public class DelegatingRestHandler implements RestHandler { + + protected final RestHandler delegate; + + public DelegatingRestHandler(RestHandler delegate) { + Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); + this.delegate = delegate; + } + + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + delegate.handleRequest(request, channel, client); + } + + @Override + public boolean canTripCircuitBreaker() { + return delegate.canTripCircuitBreaker(); + } + + @Override + public boolean supportsContentStream() { + return delegate.supportsContentStream(); + } + + @Override + public boolean allowsUnsafeBuffers() { + return delegate.allowsUnsafeBuffers(); + } + + @Override + public List routes() { + return delegate.routes(); + } + + @Override + public List deprecatedRoutes() { + return delegate.deprecatedRoutes(); + } + + @Override + public List replacedRoutes() { + return delegate.replacedRoutes(); + } + + @Override + public boolean allowSystemIndexAccessByDefault() { + return delegate.allowSystemIndexAccessByDefault(); + } + + @Override + public String toString() { + return delegate.toString(); + } + + @Override + public boolean supportsStreaming() { + return delegate.supportsStreaming(); + } +} 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 77cab13880c27..e3110c6c2ab29 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 @@ -12,16 +12,25 @@ import org.apache.logging.log4j.Logger; import org.apache.shiro.SecurityUtils; import org.apache.shiro.mgt.SecurityManager; +import org.opensearch.client.node.NodeClient; import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.concurrent.ThreadContext; +import org.opensearch.core.rest.RestStatus; import org.opensearch.identity.Subject; +import org.opensearch.identity.tokens.AuthToken; +import org.opensearch.identity.tokens.RestTokenExtractor; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.plugins.Plugin; +import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestHandler; +import org.opensearch.rest.RestRequest; + +import java.util.function.UnaryOperator; /** * Identity implementation with Shiro - * - * @opensearch.experimental */ public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin { private Logger log = LogManager.getLogger(this.getClass()); @@ -61,4 +70,35 @@ public Subject getSubject() { public TokenManager getTokenManager() { return this.authTokenHandler; } + + @Override + public UnaryOperator authenticate(ThreadContext threadContext) { + return AuthcRestHandler::new; + } + + class AuthcRestHandler extends DelegatingRestHandler { + public AuthcRestHandler(RestHandler original) { + super(original); + } + + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + final AuthToken token = RestTokenExtractor.extractToken(request); + // If no token was found, continue executing the request + if (token == null) { + // Authentication did not fail so return true. Authorization is handled at the action level. + delegate.handleRequest(request, channel, client); + return; + } + try { + ShiroSubject shiroSubject = (ShiroSubject) getSubject(); + shiroSubject.authenticate(token); + // Caller was authorized, forward the request to the handler + delegate.handleRequest(request, channel, client); + } catch (final Exception e) { + final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, e.getMessage()); + channel.sendResponse(bytesRestResponse); + } + } + } } diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSecurityManager.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSecurityManager.java index 96cf05ac53a1a..0a809dd6c9071 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSecurityManager.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSecurityManager.java @@ -15,8 +15,6 @@ /** * OpenSearch specific security manager implementation - * - * @opensearch.experimental */ public class ShiroSecurityManager extends DefaultSecurityManager { diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java index e55204593621c..cf244b822b825 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java @@ -16,8 +16,6 @@ /** * Subject backed by Shiro - * - * @opensearch.experimental */ public class ShiroSubject implements Subject { private final ShiroTokenManager authTokenHandler; diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenManager.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenManager.java index a14215aa7655b..cd54bbf9b3124 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenManager.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenManager.java @@ -36,8 +36,6 @@ /** * Extracts Shiro's {@link AuthenticationToken} from different types of auth headers - * - * @opensearch.experimental */ class ShiroTokenManager implements TokenManager { diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java index a2cb78425929e..f8113101deb70 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java @@ -16,8 +16,6 @@ /** * Password matcher for BCrypt - * - * @opensearch.experimental */ public class BCryptPasswordMatcher implements CredentialsMatcher { diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java index ef405a5637ae7..21440139d1159 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java @@ -25,8 +25,6 @@ /** * Internal Realm is a custom realm using the internal OpenSearch IdP - * - * @opensearch.experimental */ public class OpenSearchRealm extends AuthenticatingRealm { private static final String DEFAULT_REALM_NAME = "internal"; diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/User.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/User.java index 35b3348a955d7..1d2d0fed800e2 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/User.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/User.java @@ -12,8 +12,6 @@ /** * A non-volatile and immutable object in the storage. - * - * @opensearch.experimental */ public class User { 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 626cd44d13ec8..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; @@ -20,13 +21,14 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; 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, pluginList1); + IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList1); assertThat(identityService1.getTokenManager(), is(instanceOf(ShiroTokenManager.class))); } @@ -35,7 +37,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, 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/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index c86e6580122d5..e972190dd9e5c 100644 --- a/server/src/main/java/org/opensearch/action/ActionModule.java +++ b/server/src/main/java/org/opensearch/action/ActionModule.java @@ -480,6 +480,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListSet; @@ -491,6 +492,7 @@ import static java.util.Collections.unmodifiableMap; import static java.util.Objects.requireNonNull; +import static org.opensearch.rest.RestController.PASS_THROUGH_REST_HANDLER_WRAPPER; /** * Builds and binds the generic action map, all {@link TransportAction}s, and {@link ActionFilters}. @@ -522,6 +524,7 @@ public class ActionModule extends AbstractModule { private final AutoCreateIndex autoCreateIndex; private final DestructiveOperations destructiveOperations; private final RestController restController; + private final UnaryOperator restWrapper; private final RequestValidators mappingRequestValidators; private final RequestValidators indicesAliasesRequestRequestValidators; private final ThreadPool threadPool; @@ -559,17 +562,21 @@ public ActionModule( actionPlugins.stream().flatMap(p -> p.getRestHeaders().stream()), Stream.of(new RestHeaderDefinition(Task.X_OPAQUE_ID, false)) ).collect(Collectors.toSet()); - UnaryOperator restWrapper = null; - for (ActionPlugin plugin : actionPlugins) { - UnaryOperator newRestWrapper = plugin.getRestHandlerWrapper(threadPool.getThreadContext()); - if (newRestWrapper != null) { - logger.debug("Using REST wrapper from plugin " + plugin.getClass().getName()); - if (restWrapper != null) { + UnaryOperator restWrapper = identityService.authenticate(); + // Check if implementation is provided by one of the actionPlugins + if (PASS_THROUGH_REST_HANDLER_WRAPPER.equals(restWrapper)) { + List> restWrappers = actionPlugins.stream() + .map(p -> p.getRestHandlerWrapper(threadPool.getThreadContext())) + .filter(Objects::nonNull) + .collect(Collectors.toUnmodifiableList()); + if (!restWrappers.isEmpty()) { + if (restWrappers.size() > 1) { throw new IllegalArgumentException("Cannot have more than one plugin implementing a REST wrapper"); } - restWrapper = newRestWrapper; + restWrapper = restWrappers.get(0); } } + this.restWrapper = restWrapper; mappingRequestValidators = new RequestValidators<>( actionPlugins.stream().flatMap(p -> p.mappingRequestValidators().stream()).collect(Collectors.toList()) ); @@ -577,7 +584,7 @@ public ActionModule( actionPlugins.stream().flatMap(p -> p.indicesAliasesRequestValidators().stream()).collect(Collectors.toList()) ); - restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService, identityService); + restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); } public Map> getActions() { @@ -1059,6 +1066,11 @@ public RestController getRestController() { return restController; } + // Visible for testing + UnaryOperator getRestWrapper() { + return restWrapper; + } + /** * The DynamicActionRegistry maintains a registry mapping {@link ActionType} instances to {@link TransportAction} instances. *

diff --git a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java index 81b1b91b11481..5bc655af4df7b 100644 --- a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java @@ -20,7 +20,6 @@ import org.opensearch.transport.TransportService; import java.io.IOException; -import java.util.List; import java.util.Optional; import java.util.Set; @@ -31,8 +30,8 @@ */ public class NoopExtensionsManager extends ExtensionsManager { - public NoopExtensionsManager() throws IOException { - super(Set.of(), new IdentityService(Settings.EMPTY, List.of())); + public NoopExtensionsManager(IdentityService identityService) throws IOException { + super(Set.of(), identityService); } @Override diff --git a/server/src/main/java/org/opensearch/identity/IdentityService.java b/server/src/main/java/org/opensearch/identity/IdentityService.java index 3129c201b9a39..f19411e41c8c0 100644 --- a/server/src/main/java/org/opensearch/identity/IdentityService.java +++ b/server/src/main/java/org/opensearch/identity/IdentityService.java @@ -12,8 +12,11 @@ import org.opensearch.identity.noop.NoopIdentityPlugin; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.IdentityPlugin; +import org.opensearch.rest.RestHandler; +import org.opensearch.threadpool.ThreadPool; import java.util.List; +import java.util.function.UnaryOperator; import java.util.stream.Collectors; /** @@ -26,9 +29,11 @@ public class IdentityService { private final Settings settings; private final IdentityPlugin identityPlugin; + private final ThreadPool threadPool; - public IdentityService(final Settings settings, final List identityPlugins) { + public IdentityService(final Settings settings, final ThreadPool threadPool, final List identityPlugins) { this.settings = settings; + this.threadPool = threadPool; if (identityPlugins.size() == 0) { log.debug("Identity plugins size is 0"); @@ -57,4 +62,11 @@ public Subject getSubject() { public TokenManager getTokenManager() { return identityPlugin.getTokenManager(); } + + /** + * Gets the RestHandlerWrapper to authenticate the request + */ + public UnaryOperator authenticate() { + return identityPlugin.authenticate(this.threadPool.getThreadContext()); + } } diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 1a9b233b387b2..d9e09c243223a 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -525,19 +525,6 @@ protected Node( identityPlugins.addAll(pluginsService.filterPlugins(IdentityPlugin.class)); } - final IdentityService identityService = new IdentityService(settings, 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(); - } - final Set additionalRoles = pluginsService.filterPlugins(Plugin.class) .stream() .map(Plugin::getRoles) @@ -575,6 +562,19 @@ 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); diff --git a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java index 410535504f0dd..f5519cf1c144e 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java @@ -8,8 +8,14 @@ package org.opensearch.plugins; +import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; +import org.opensearch.rest.RestHandler; + +import java.util.function.UnaryOperator; + +import static org.opensearch.rest.RestController.PASS_THROUGH_REST_HANDLER_WRAPPER; /** * Plugin that provides identity and access control for OpenSearch @@ -22,11 +28,20 @@ public interface IdentityPlugin { * Get the current subject. * @return Should never return null * */ - public Subject getSubject(); + Subject getSubject(); /** * Get the Identity Plugin's token manager implementation * @return Should never return null. */ - public TokenManager getTokenManager(); + TokenManager getTokenManager(); + + /** + * Returns a function used to wrap each rest request before handling the request. + * The returned {@link UnaryOperator} is called for every incoming rest request and receives + * the original rest handler as it's input. + */ + default UnaryOperator authenticate(ThreadContext threadContext) { + return PASS_THROUGH_REST_HANDLER_WRAPPER; + } } diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 7d0c1e2260de1..0afbce7929203 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -41,7 +41,6 @@ import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.common.path.PathTrie; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.common.util.io.Streams; import org.opensearch.common.xcontent.XContentType; @@ -56,10 +55,6 @@ import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.http.HttpChunk; import org.opensearch.http.HttpServerTransport; -import org.opensearch.identity.IdentityService; -import org.opensearch.identity.Subject; -import org.opensearch.identity.tokens.AuthToken; -import org.opensearch.identity.tokens.RestTokenExtractor; import org.opensearch.usage.UsageService; import java.io.ByteArrayOutputStream; @@ -115,6 +110,8 @@ public class RestController implements HttpServerTransport.Dispatcher { private final PathTrie handlers = new PathTrie<>(RestUtils.REST_DECODER); + public static final UnaryOperator PASS_THROUGH_REST_HANDLER_WRAPPER = (h) -> h; + private final UnaryOperator handlerWrapper; private final NodeClient client; @@ -124,25 +121,19 @@ public class RestController implements HttpServerTransport.Dispatcher { /** Rest headers that are copied to internal requests made during a rest request. */ private final Set headersToCopy; private final UsageService usageService; - private final IdentityService identityService; public RestController( Set headersToCopy, UnaryOperator handlerWrapper, NodeClient client, CircuitBreakerService circuitBreakerService, - UsageService usageService, - IdentityService identityService + UsageService usageService ) { this.headersToCopy = headersToCopy; this.usageService = usageService; - if (handlerWrapper == null) { - handlerWrapper = h -> h; // passthrough if no wrapper set - } this.handlerWrapper = handlerWrapper; this.client = client; this.circuitBreakerService = circuitBreakerService; - this.identityService = identityService; registerHandlerNoWrap( RestRequest.Method.GET, "/favicon.ico", @@ -465,11 +456,6 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel return; } } else { - if (FeatureFlags.isEnabled(FeatureFlags.IDENTITY)) { - if (!handleAuthenticateUser(request, channel)) { - return; - } - } dispatchRequest(request, channel, handler); return; } @@ -580,41 +566,6 @@ private void handleBadRequest(String uri, RestRequest.Method method, RestChannel } } - /** - * Attempts to extract auth token and login. - * - * @return false if there was an error and the request should not continue being dispatched - * */ - private boolean handleAuthenticateUser(final RestRequest request, final RestChannel channel) { - try { - final AuthToken token = RestTokenExtractor.extractToken(request); - // If no token was found, continue executing the request - if (token == null) { - // Authentication did not fail so return true. Authorization is handled at the action level. - return true; - } - final Subject currentSubject = identityService.getSubject(); - currentSubject.authenticate(token); - logger.debug("Logged in as user " + currentSubject); - } catch (final Exception e) { - try { - final BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse( - channel, - RestStatus.UNAUTHORIZED, - e.getMessage() - ); - channel.sendResponse(bytesRestResponse); - } catch (final Exception ex) { - final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, ex.getMessage()); - channel.sendResponse(bytesRestResponse); - } - return false; - } - - // Authentication did not fail so return true. Authorization is handled at the action level. - return true; - } - /** * Get the valid set of HTTP methods for a REST request. */ diff --git a/server/src/test/java/org/opensearch/action/ActionModuleTests.java b/server/src/test/java/org/opensearch/action/ActionModuleTests.java index 8479f011adf48..c7b3e3aca8d9d 100644 --- a/server/src/test/java/org/opensearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/opensearch/action/ActionModuleTests.java @@ -49,8 +49,11 @@ import org.opensearch.core.action.ActionResponse; import org.opensearch.extensions.ExtensionsManager; import org.opensearch.identity.IdentityService; +import org.opensearch.identity.Subject; +import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.ActionPlugin; import org.opensearch.plugins.ActionPlugin.ActionHandler; +import org.opensearch.plugins.IdentityPlugin; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestController; import org.opensearch.rest.RestHandler; @@ -69,11 +72,17 @@ import java.util.List; import java.util.Set; import java.util.function.Supplier; +import java.util.function.UnaryOperator; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; +import static org.opensearch.rest.RestController.PASS_THROUGH_REST_HANDLER_WRAPPER; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.startsWith; +import static org.mockito.Mockito.mock; public class ActionModuleTests extends OpenSearchTestCase { public void testSetupActionsContainsKnownBuiltin() { @@ -142,8 +151,8 @@ public void testSetupRestHandlerContainsKnownBuiltin() throws IOException { null, usageService, null, - new IdentityService(Settings.EMPTY, new ArrayList<>()), - new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, 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 @@ -160,6 +169,119 @@ public List routes() { }) ); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); + assertThat(actionModule.getRestWrapper(), equalTo(PASS_THROUGH_REST_HANDLER_WRAPPER)); + } + + private class TestIdentityPlugin implements IdentityPlugin { + + @Override + public Subject getSubject() { + return null; + } + + @Override + public TokenManager getTokenManager() { + return null; + } + + @Override + public UnaryOperator authenticate(ThreadContext threadContext) { + return (h) -> { + threadContext.putHeader("test_header", "foo"); + return h; + }; + } + } + + private class TestActionPlugin implements ActionPlugin { + + private ThreadPool threadPool; + + public TestActionPlugin(ThreadPool threadPool) { + this.threadPool = threadPool; + } + + @Override + public UnaryOperator getRestHandlerWrapper(ThreadContext threadContext) { + return (h) -> { + threadContext.putHeader("test_header", "bar"); + return h; + }; + } + } + + public void testSetupRestHandlerWithIdentityPlugin() throws IOException { + ThreadPool threadPool = mock(ThreadPool.class); + IdentityPlugin testIdentityPlugin = new TestIdentityPlugin(); + SettingsModule settings = new SettingsModule(Settings.EMPTY); + UsageService usageService = new UsageService(); + IdentityService identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of(testIdentityPlugin)); + ActionModule actionModule = new ActionModule( + settings.getSettings(), + new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), + settings.getIndexScopedSettings(), + settings.getClusterSettings(), + settings.getSettingsFilter(), + threadPool, + emptyList(), + null, + null, + usageService, + null, + identityService, + new ExtensionsManager(Set.of(), identityService) + ); + assertThat(actionModule.getRestWrapper(), not(equalTo(PASS_THROUGH_REST_HANDLER_WRAPPER))); + } + + public void testSetupRestHandlerWithActionPluginNoIdentityPlugin() throws IOException { + ThreadPool threadPool = mock(ThreadPool.class); + TestActionPlugin testActionPlugin = new TestActionPlugin(threadPool); + SettingsModule settings = new SettingsModule(Settings.EMPTY); + UsageService usageService = new UsageService(); + ActionModule actionModule = new ActionModule( + settings.getSettings(), + new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), + settings.getIndexScopedSettings(), + settings.getClusterSettings(), + settings.getSettingsFilter(), + threadPool, + List.of(testActionPlugin), + null, + null, + usageService, + null, + new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()), + new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of())) + ); + assertThat(actionModule.getRestWrapper(), not(equalTo(PASS_THROUGH_REST_HANDLER_WRAPPER))); + } + + public void testSetupRestHandlerWithIdentityPluginOverrideActionPlugin() throws IOException { + IdentityPlugin testIdentityPlugin = new TestIdentityPlugin(); + ThreadPool threadPool = mock(ThreadPool.class); + TestActionPlugin testActionPlugin = new TestActionPlugin(threadPool); + SettingsModule settings = new SettingsModule(Settings.EMPTY); + UsageService usageService = new UsageService(); + IdentityService identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of(testIdentityPlugin)); + ActionModule actionModule = new ActionModule( + settings.getSettings(), + new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), + settings.getIndexScopedSettings(), + settings.getClusterSettings(), + settings.getSettingsFilter(), + threadPool, + List.of(testActionPlugin), + null, + null, + usageService, + null, + identityService, + new ExtensionsManager(Set.of(), identityService) + ); + assertThat(actionModule.getRestWrapper(), not(equalTo(PASS_THROUGH_REST_HANDLER_WRAPPER))); + assertThat(actionModule.getRestWrapper().getClass().getName(), not(containsString("TestActionPlugin"))); + assertThat(actionModule.getRestWrapper().getClass().getName(), containsString("TestIdentityPlugin")); } public void testPluginCantOverwriteBuiltinRestHandler() throws IOException { @@ -186,6 +308,7 @@ public String getName() { }; SettingsModule settings = new SettingsModule(Settings.EMPTY); ThreadPool threadPool = new TestThreadPool(getTestName()); + IdentityService identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()); try { UsageService usageService = new UsageService(); ActionModule actionModule = new ActionModule( @@ -200,7 +323,7 @@ public String getName() { null, usageService, null, - null, + identityService, null ); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); @@ -237,6 +360,7 @@ public List getRestHandlers( SettingsModule settings = new SettingsModule(Settings.EMPTY); ThreadPool threadPool = new TestThreadPool(getTestName()); + IdentityService identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()); try { UsageService usageService = new UsageService(); ActionModule actionModule = new ActionModule( @@ -251,7 +375,7 @@ public List getRestHandlers( null, usageService, null, - null, + identityService, null ); actionModule.initRestHandlers(null); diff --git a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java index 2129810a99879..c4e43af3d63b4 100644 --- a/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java +++ b/server/src/test/java/org/opensearch/bootstrap/IdentityPluginTests.java @@ -15,18 +15,20 @@ import org.opensearch.identity.noop.NoopTokenManager; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.test.OpenSearchTestCase; +import org.opensearch.threadpool.ThreadPool; import java.util.List; 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() { IdentityPlugin identityPlugin1 = new NoopIdentityPlugin(); List pluginList1 = List.of(identityPlugin1); - IdentityService identityService1 = new IdentityService(Settings.EMPTY, pluginList1); + IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList1); assertTrue(identityService1.getSubject().getPrincipal().getName().equalsIgnoreCase("Unauthenticated")); assertThat(identityService1.getTokenManager(), is(instanceOf(NoopTokenManager.class))); } @@ -36,7 +38,10 @@ public void testMultipleIdentityPluginsFail() { IdentityPlugin identityPlugin2 = new NoopIdentityPlugin(); IdentityPlugin identityPlugin3 = new NoopIdentityPlugin(); List pluginList = List.of(identityPlugin1, identityPlugin2, identityPlugin3); - Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, 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/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 3c25dbdff3342..bf1d52b49cb1f 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -153,8 +153,7 @@ public List> getExtensionSettings() { null, new NodeClient(Settings.EMPTY, threadPool), new NoneCircuitBreakerService(), - new UsageService(), - new IdentityService(Settings.EMPTY, List.of()) + new UsageService() ); when(actionModule.getDynamicActionRegistry()).thenReturn(mock(DynamicActionRegistry.class)); when(actionModule.getRestController()).thenReturn(restController); @@ -171,7 +170,7 @@ public List> getExtensionSettings() { Collections.emptyList() ); client = new NoOpNodeClient(this.getTestName()); - identityService = new IdentityService(Settings.EMPTY, 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 8b73f2e81972f..c0158a347a7c2 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/ExtensionRestRequestTests.java @@ -29,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; @@ -38,6 +39,7 @@ import java.util.Map; import static java.util.Map.entry; +import static org.mockito.Mockito.mock; public class ExtensionRestRequestTests extends OpenSearchTestCase { @@ -72,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, List.of()); + identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()); TokenManager tokenManager = identityService.getTokenManager(); Subject subject = this.identityService.getSubject(); 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 0dae0ae1b4e0b..ac818c3bb4a7b 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestInitializeExtensionActionTests.java @@ -121,7 +121,7 @@ public void testRestInitializeExtensionActionResponse() throws Exception { } public void testRestInitializeExtensionActionFailure() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, 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\"," @@ -156,7 +156,7 @@ public void testRestInitializeExtensionActionResponseWithAdditionalSettings() th ); ExtensionsManager extensionsManager = new ExtensionsManager( Set.of(boolSetting, stringSetting, intSetting, listSetting), - new IdentityService(Settings.EMPTY, List.of()) + new IdentityService(Settings.EMPTY, threadPool, List.of()) ); ExtensionsManager spy = spy(extensionsManager); @@ -206,7 +206,7 @@ public void testRestInitializeExtensionActionResponseWithAdditionalSettingsUsing ); ExtensionsManager extensionsManager = new ExtensionsManager( Set.of(boolSetting, stringSetting, intSetting, listSetting), - new IdentityService(Settings.EMPTY, 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 9da976de7d7f6..e9c910ea361fb 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java @@ -122,10 +122,10 @@ public void setup() throws Exception { null, usageService, null, - new IdentityService(Settings.EMPTY, new ArrayList<>()), - new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, 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, new ArrayList<>()); + identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()); dynamicActionRegistry = actionModule.getDynamicActionRegistry(); } diff --git a/server/src/test/java/org/opensearch/rest/RestControllerTests.java b/server/src/test/java/org/opensearch/rest/RestControllerTests.java index b7239e7b59742..f7f1b02847854 100644 --- a/server/src/test/java/org/opensearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/opensearch/rest/RestControllerTests.java @@ -55,7 +55,6 @@ import org.opensearch.http.HttpResponse; import org.opensearch.http.HttpServerTransport; import org.opensearch.http.HttpStats; -import org.opensearch.identity.IdentityService; import org.opensearch.indices.breaker.HierarchyCircuitBreakerService; import org.opensearch.rest.action.admin.indices.RestCreateIndexAction; import org.opensearch.test.OpenSearchTestCase; @@ -96,7 +95,6 @@ public class RestControllerTests extends OpenSearchTestCase { private RestController restController; private HierarchyCircuitBreakerService circuitBreakerService; private UsageService usageService; - private IdentityService identityService; private NodeClient client; @Before @@ -114,11 +112,9 @@ public void setup() { // we can do this here only because we know that we don't adjust breaker settings dynamically in the test inFlightRequestsBreaker = circuitBreakerService.getBreaker(CircuitBreaker.IN_FLIGHT_REQUESTS); - identityService = new IdentityService(Settings.EMPTY, List.of()); - HttpServerTransport httpServerTransport = new TestHttpServerTransport(); client = new NoOpNodeClient(this.getTestName()); - restController = new RestController(Collections.emptySet(), null, client, circuitBreakerService, usageService, identityService); + restController = new RestController(Collections.emptySet(), null, client, circuitBreakerService, usageService); restController.registerHandler( RestRequest.Method.GET, "/", @@ -139,7 +135,7 @@ public void teardown() throws IOException { } public void testDefaultRestControllerGetAllHandlersContainsFavicon() { - final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService, identityService); + final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService); Iterator handlers = restController.getAllHandlers(); assertTrue(handlers.hasNext()); MethodHandlers faviconHandler = handlers.next(); @@ -149,7 +145,7 @@ public void testDefaultRestControllerGetAllHandlersContainsFavicon() { } public void testRestControllerGetAllHandlers() { - final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService, identityService); + final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService); restController.registerHandler(RestRequest.Method.PATCH, "/foo", mock(RestHandler.class)); restController.registerHandler(RestRequest.Method.GET, "/foo", mock(RestHandler.class)); @@ -174,7 +170,7 @@ public void testApplyRelevantHeaders() throws Exception { Set headers = new HashSet<>( Arrays.asList(new RestHeaderDefinition("header.1", true), new RestHeaderDefinition("header.2", true)) ); - final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService, identityService); + final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService); Map> restHeaders = new HashMap<>(); restHeaders.put("header.1", Collections.singletonList("true")); restHeaders.put("header.2", Collections.singletonList("true")); @@ -210,7 +206,7 @@ public void testRequestWithDisallowedMultiValuedHeader() { Set headers = new HashSet<>( Arrays.asList(new RestHeaderDefinition("header.1", true), new RestHeaderDefinition("header.2", false)) ); - final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService, identityService); + final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService); Map> restHeaders = new HashMap<>(); restHeaders.put("header.1", Collections.singletonList("boo")); restHeaders.put("header.2", Arrays.asList("foo", "bar")); @@ -225,14 +221,7 @@ public void testRequestWithDisallowedMultiValuedHeaderButSameValues() { Set headers = new HashSet<>( Arrays.asList(new RestHeaderDefinition("header.1", true), new RestHeaderDefinition("header.2", false)) ); - final RestController restController = new RestController( - headers, - null, - client, - circuitBreakerService, - usageService, - identityService - ); + final RestController restController = new RestController(headers, null, client, circuitBreakerService, usageService); Map> restHeaders = new HashMap<>(); restHeaders.put("header.1", Collections.singletonList("boo")); restHeaders.put("header.2", Arrays.asList("foo", "foo")); @@ -293,7 +282,7 @@ public void testRegisterWithDeprecatedHandler() { } public void testRegisterSecondMethodWithDifferentNamedWildcard() { - final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService, identityService); + final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService); RestRequest.Method firstMethod = randomFrom(RestRequest.Method.values()); RestRequest.Method secondMethod = randomFrom( @@ -321,7 +310,7 @@ public void testRestHandlerWrapper() throws Exception { final RestController restController = new RestController(Collections.emptySet(), h -> { assertSame(handler, h); return (RestRequest request, RestChannel channel, NodeClient client) -> wrapperCalled.set(true); - }, client, circuitBreakerService, usageService, identityService); + }, client, circuitBreakerService, usageService); restController.registerHandler(RestRequest.Method.GET, "/wrapped", handler); RestRequest request = testRestRequest("/wrapped", "{}", MediaTypeRegistry.JSON); AssertingChannel channel = new AssertingChannel(request, true, RestStatus.BAD_REQUEST); @@ -384,7 +373,7 @@ public void testDispatchRequiresContentTypeForRequestsWithContent() { String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead())); RestRequest request = testRestRequest("/", content, null); AssertingChannel channel = new AssertingChannel(request, true, RestStatus.NOT_ACCEPTABLE); - restController = new RestController(Collections.emptySet(), null, null, circuitBreakerService, usageService, identityService); + restController = new RestController(Collections.emptySet(), null, null, circuitBreakerService, usageService); restController.registerHandler( RestRequest.Method.GET, "/", diff --git a/server/src/test/java/org/opensearch/rest/RestHttpResponseHeadersTests.java b/server/src/test/java/org/opensearch/rest/RestHttpResponseHeadersTests.java index 5d677247b8b6d..b8602cdc20e6a 100644 --- a/server/src/test/java/org/opensearch/rest/RestHttpResponseHeadersTests.java +++ b/server/src/test/java/org/opensearch/rest/RestHttpResponseHeadersTests.java @@ -39,7 +39,6 @@ import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.indices.breaker.CircuitBreakerService; import org.opensearch.core.rest.RestStatus; -import org.opensearch.identity.IdentityService; import org.opensearch.indices.breaker.HierarchyCircuitBreakerService; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.rest.FakeRestChannel; @@ -104,17 +103,8 @@ public void testUnsupportedMethodResponseHttpHeader() throws Exception { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS) ); - final Settings settings = Settings.EMPTY; UsageService usageService = new UsageService(); - final IdentityService identityService = new IdentityService(settings, List.of()); - RestController restController = new RestController( - Collections.emptySet(), - null, - null, - circuitBreakerService, - usageService, - identityService - ); + RestController restController = new RestController(Collections.emptySet(), null, null, circuitBreakerService, usageService); // A basic RestHandler handles requests to the endpoint RestHandler restHandler = new RestHandler() { diff --git a/server/src/test/java/org/opensearch/rest/action/admin/indices/RestValidateQueryActionTests.java b/server/src/test/java/org/opensearch/rest/action/admin/indices/RestValidateQueryActionTests.java index 3fb6764846da6..6aa1d10d71e50 100644 --- a/server/src/test/java/org/opensearch/rest/action/admin/indices/RestValidateQueryActionTests.java +++ b/server/src/test/java/org/opensearch/rest/action/admin/indices/RestValidateQueryActionTests.java @@ -44,7 +44,6 @@ import org.opensearch.core.common.io.stream.NamedWriteableRegistry; import org.opensearch.core.indices.breaker.NoneCircuitBreakerService; import org.opensearch.core.xcontent.MediaTypeRegistry; -import org.opensearch.identity.IdentityService; import org.opensearch.rest.RestController; import org.opensearch.rest.RestRequest; import org.opensearch.search.AbstractSearchTestCase; @@ -61,7 +60,6 @@ import java.util.Collections; import java.util.HashMap; -import java.util.List; import java.util.Map; import static java.util.Collections.emptyMap; @@ -75,15 +73,7 @@ public class RestValidateQueryActionTests extends AbstractSearchTestCase { private static NodeClient client = new NodeClient(Settings.EMPTY, threadPool); private static UsageService usageService = new UsageService(); - private static IdentityService identityService = new IdentityService(Settings.EMPTY, List.of()); - private static RestController controller = new RestController( - emptySet(), - null, - client, - new NoneCircuitBreakerService(), - usageService, - identityService - ); + private static RestController controller = new RestController(emptySet(), null, client, new NoneCircuitBreakerService(), usageService); private static RestValidateQueryAction action = new RestValidateQueryAction(); /** diff --git a/test/framework/src/main/java/org/opensearch/test/rest/RestActionTestCase.java b/test/framework/src/main/java/org/opensearch/test/rest/RestActionTestCase.java index a77865579f3b3..fec1699c9ef64 100644 --- a/test/framework/src/main/java/org/opensearch/test/rest/RestActionTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/rest/RestActionTestCase.java @@ -40,7 +40,6 @@ import org.opensearch.core.action.ActionListener; import org.opensearch.core.action.ActionResponse; import org.opensearch.core.indices.breaker.NoneCircuitBreakerService; -import org.opensearch.identity.IdentityService; import org.opensearch.rest.RestController; import org.opensearch.rest.RestRequest; import org.opensearch.tasks.Task; @@ -52,7 +51,6 @@ import org.junit.Before; import java.util.Collections; -import java.util.List; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiFunction; @@ -67,15 +65,7 @@ public abstract class RestActionTestCase extends OpenSearchTestCase { @Before public void setUpController() { verifyingClient = new VerifyingClient(this.getTestName()); - final IdentityService identityService = new IdentityService(Settings.EMPTY, List.of()); - controller = new RestController( - Collections.emptySet(), - null, - verifyingClient, - new NoneCircuitBreakerService(), - new UsageService(), - identityService - ); + controller = new RestController(Collections.emptySet(), null, verifyingClient, new NoneCircuitBreakerService(), new UsageService()); } @After From bc610a25b581abdb65618625224acb0372fb9161 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 26 Aug 2024 20:16:02 -0400 Subject: [PATCH 02/15] Handle null Signed-off-by: Craig Perkins --- .../src/main/java/org/opensearch/rest/RestController.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 0afbce7929203..0773d9d0fda03 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -131,7 +131,11 @@ public RestController( ) { this.headersToCopy = headersToCopy; this.usageService = usageService; - this.handlerWrapper = handlerWrapper; + if (handlerWrapper == null) { + this.handlerWrapper = PASS_THROUGH_REST_HANDLER_WRAPPER; + } else { + this.handlerWrapper = handlerWrapper; + } this.client = client; this.circuitBreakerService = circuitBreakerService; registerHandlerNoWrap( From 1abfe9747245c6bcde568a2a67cc3d3d01eaef33 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 26 Aug 2024 21:37:58 -0400 Subject: [PATCH 03/15] Fix tests Signed-off-by: Craig Perkins --- .../identity/shiro/ShiroIdentityPlugin.java | 14 +++++++------- .../identity/shiro/realm/OpenSearchRealm.java | 4 ++-- 2 files changed, 9 insertions(+), 9 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 e3110c6c2ab29..fd92f9def421f 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 @@ -83,14 +83,14 @@ public AuthcRestHandler(RestHandler original) { @Override public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - final AuthToken token = RestTokenExtractor.extractToken(request); - // If no token was found, continue executing the request - if (token == null) { - // Authentication did not fail so return true. Authorization is handled at the action level. - delegate.handleRequest(request, channel, client); - return; - } try { + final AuthToken token = RestTokenExtractor.extractToken(request); + // If no token was found, continue executing the request + if (token == null) { + // Authentication did not fail so return true. Authorization is handled at the action level. + delegate.handleRequest(request, channel, client); + return; + } ShiroSubject shiroSubject = (ShiroSubject) getSubject(); shiroSubject.authenticate(token); // Caller was authorized, forward the request to the handler diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java index 21440139d1159..1fc9a1f437a42 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java @@ -91,7 +91,7 @@ public OpenSearchRealm build() { public User getInternalUser(final String principalIdentifier) throws UnknownAccountException { final User userRecord = internalUsers.get(principalIdentifier); if (userRecord == null) { - throw new UnknownAccountException(); + throw new UnknownAccountException("Incorrect credentials"); } return userRecord; } @@ -129,7 +129,7 @@ protected AuthenticationInfo doGetAuthenticationInfo(final AuthenticationToken t return sai; } else { // Bad password - throw new IncorrectCredentialsException(); + throw new IncorrectCredentialsException("Incorrect credentials"); } } From cdf1892cd87a554a0b4744db53d520263bc0fdcb Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 28 Aug 2024 15:46:23 -0400 Subject: [PATCH 04/15] Fix ActionModuleTests Signed-off-by: Craig Perkins --- .../java/org/opensearch/action/ActionModuleTests.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/action/ActionModuleTests.java b/server/src/test/java/org/opensearch/action/ActionModuleTests.java index c7b3e3aca8d9d..8fddf84cb0597 100644 --- a/server/src/test/java/org/opensearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/opensearch/action/ActionModuleTests.java @@ -49,11 +49,13 @@ import org.opensearch.core.action.ActionResponse; import org.opensearch.extensions.ExtensionsManager; import org.opensearch.identity.IdentityService; +import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.ActionPlugin; import org.opensearch.plugins.ActionPlugin.ActionHandler; import org.opensearch.plugins.IdentityPlugin; +import org.opensearch.plugins.Plugin; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestController; import org.opensearch.rest.RestHandler; @@ -175,7 +177,7 @@ public List routes() { private class TestIdentityPlugin implements IdentityPlugin { @Override - public Subject getSubject() { + public Subject getCurrentSubject() { return null; } @@ -191,6 +193,11 @@ public UnaryOperator authenticate(ThreadContext threadContext) { return h; }; } + + @Override + public PluginSubject getPluginSubject(Plugin plugin) { + return null; + } } private class TestActionPlugin implements ActionPlugin { From 09e8fc0163fbb3aade29d1b921d0121a2c31ae78 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 28 Aug 2024 16:19:06 -0400 Subject: [PATCH 05/15] 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 fb89dd2723b62..5ed4dc911590a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add concurrent search support for Derived Fields ([#15326](https://github.com/opensearch-project/OpenSearch/pull/15326)) - [Workload Management] Add query group stats constructs ([#15343](https://github.com/opensearch-project/OpenSearch/pull/15343))) - Add runAs to Subject interface and introduce IdentityAwarePlugin extension point ([#14630](https://github.com/opensearch-project/OpenSearch/pull/14630)) +- Add authenticate to IdentityPlugin interface ([#15430](https://github.com/opensearch-project/OpenSearch/pull/15430)) ### Dependencies - Bump `netty` from 4.1.111.Final to 4.1.112.Final ([#15081](https://github.com/opensearch-project/OpenSearch/pull/15081)) From 97dd9352de6e77ded505230395a27f725a10739e Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 29 Aug 2024 12:55:41 -0400 Subject: [PATCH 06/15] Add DelegatingRestHandlerTests Signed-off-by: Craig Perkins --- .../shiro/DelegatingRestHandlerTests.java | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java diff --git a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java new file mode 100644 index 0000000000000..ce20b57361b12 --- /dev/null +++ b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java @@ -0,0 +1,66 @@ +/* + * 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.client.node.NodeClient; +import org.opensearch.core.common.bytes.BytesArray; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestHandler; +import org.opensearch.rest.RestRequest; +import org.junit.Test; + +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; + +public class DelegatingRestHandlerTests { + + @Test + public void testDelegatingRestHandlerShouldActAsOriginal() throws Exception { + RestHandler rh = new RestHandler() { + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); + } + }; + RestHandler handlerSpy = spy(rh); + DelegatingRestHandler drh = new DelegatingRestHandler(handlerSpy); + + List overridableMethods = Arrays.stream(RestHandler.class.getMethods()) + .filter( + m -> !(Modifier.isPrivate(m.getModifiers()) || Modifier.isStatic(m.getModifiers()) || Modifier.isFinal(m.getModifiers())) + ) + .collect(Collectors.toList()); + + for (Method method : overridableMethods) { + int argCount = method.getParameterCount(); + Object[] args = new Object[argCount]; + for (int i = 0; i < argCount; i++) { + args[i] = any(); + } + if (args.length > 0) { + method.invoke(drh, args); + } else { + method.invoke(drh); + } + method.invoke(verify(handlerSpy, times(1)), args); + } + verifyNoMoreInteractions(handlerSpy); + } +} From 74818684b1c69c7862921ce271e5eb57c370b164 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 29 Aug 2024 13:46:34 -0400 Subject: [PATCH 07/15] Address forbiddenApi Signed-off-by: Craig Perkins --- .../identity/shiro/DelegatingRestHandlerTests.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java index ce20b57361b12..b376e68622e72 100644 --- a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java +++ b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java @@ -15,7 +15,7 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestHandler; import org.opensearch.rest.RestRequest; -import org.junit.Test; +import org.opensearch.test.OpenSearchTestCase; import java.lang.reflect.Method; import java.lang.reflect.Modifier; @@ -29,9 +29,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; -public class DelegatingRestHandlerTests { +public class DelegatingRestHandlerTests extends OpenSearchTestCase { - @Test public void testDelegatingRestHandlerShouldActAsOriginal() throws Exception { RestHandler rh = new RestHandler() { @Override From 7da9bd714911f19d2ba3b0fd62959d8466bfa270 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 29 Aug 2024 15:41:24 -0400 Subject: [PATCH 08/15] Remove authenticate from IdentityPlugin and keep RestController feature flagged code removed Signed-off-by: Craig Perkins --- CHANGELOG.md | 2 +- .../identity/shiro/ShiroIdentityPlugin.java | 5 +- .../org/opensearch/action/ActionModule.java | 26 +--- .../opensearch/identity/IdentityService.java | 9 -- .../opensearch/plugins/IdentityPlugin.java | 15 -- .../org/opensearch/rest/RestController.java | 8 +- .../opensearch/action/ActionModuleTests.java | 128 ------------------ 7 files changed, 14 insertions(+), 179 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba17b562a1c55..3be3376a5ad4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Workload Management] Add query group stats constructs ([#15343](https://github.com/opensearch-project/OpenSearch/pull/15343))) - Add runAs to Subject interface and introduce IdentityAwarePlugin extension point ([#14630](https://github.com/opensearch-project/OpenSearch/pull/14630)) - Optimize NodeIndicesStats output behind flag ([#14454](https://github.com/opensearch-project/OpenSearch/pull/14454)) -- Add authenticate to IdentityPlugin interface ([#15430](https://github.com/opensearch-project/OpenSearch/pull/15430)) +- Remove identity-related feature flagged code from the RestController ([#15430](https://github.com/opensearch-project/OpenSearch/pull/15430)) ### Dependencies - Bump `netty` from 4.1.111.Final to 4.1.112.Final ([#15081](https://github.com/opensearch-project/OpenSearch/pull/15081)) 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 30e22d5ce02fa..f4700af981761 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 @@ -29,6 +29,7 @@ import org.opensearch.identity.tokens.AuthToken; import org.opensearch.identity.tokens.RestTokenExtractor; import org.opensearch.identity.tokens.TokenManager; +import org.opensearch.plugins.ActionPlugin; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.plugins.Plugin; import org.opensearch.repositories.RepositoriesService; @@ -49,7 +50,7 @@ * Identity implementation with Shiro */ @ExperimentalApi -public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin { +public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin, ActionPlugin { private Logger log = LogManager.getLogger(this.getClass()); private final Settings settings; @@ -109,7 +110,7 @@ public TokenManager getTokenManager() { } @Override - public UnaryOperator authenticate(ThreadContext threadContext) { + public UnaryOperator getRestHandlerWrapper(ThreadContext threadContext) { return AuthcRestHandler::new; } diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index e972190dd9e5c..50db09fbba05b 100644 --- a/server/src/main/java/org/opensearch/action/ActionModule.java +++ b/server/src/main/java/org/opensearch/action/ActionModule.java @@ -480,7 +480,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListSet; @@ -492,7 +491,6 @@ import static java.util.Collections.unmodifiableMap; import static java.util.Objects.requireNonNull; -import static org.opensearch.rest.RestController.PASS_THROUGH_REST_HANDLER_WRAPPER; /** * Builds and binds the generic action map, all {@link TransportAction}s, and {@link ActionFilters}. @@ -524,7 +522,6 @@ public class ActionModule extends AbstractModule { private final AutoCreateIndex autoCreateIndex; private final DestructiveOperations destructiveOperations; private final RestController restController; - private final UnaryOperator restWrapper; private final RequestValidators mappingRequestValidators; private final RequestValidators indicesAliasesRequestRequestValidators; private final ThreadPool threadPool; @@ -562,21 +559,17 @@ public ActionModule( actionPlugins.stream().flatMap(p -> p.getRestHeaders().stream()), Stream.of(new RestHeaderDefinition(Task.X_OPAQUE_ID, false)) ).collect(Collectors.toSet()); - UnaryOperator restWrapper = identityService.authenticate(); - // Check if implementation is provided by one of the actionPlugins - if (PASS_THROUGH_REST_HANDLER_WRAPPER.equals(restWrapper)) { - List> restWrappers = actionPlugins.stream() - .map(p -> p.getRestHandlerWrapper(threadPool.getThreadContext())) - .filter(Objects::nonNull) - .collect(Collectors.toUnmodifiableList()); - if (!restWrappers.isEmpty()) { - if (restWrappers.size() > 1) { + UnaryOperator restWrapper = null; + for (ActionPlugin plugin : actionPlugins) { + UnaryOperator newRestWrapper = plugin.getRestHandlerWrapper(threadPool.getThreadContext()); + if (newRestWrapper != null) { + logger.debug("Using REST wrapper from plugin " + plugin.getClass().getName()); + if (restWrapper != null) { throw new IllegalArgumentException("Cannot have more than one plugin implementing a REST wrapper"); } - restWrapper = restWrappers.get(0); + restWrapper = newRestWrapper; } } - this.restWrapper = restWrapper; mappingRequestValidators = new RequestValidators<>( actionPlugins.stream().flatMap(p -> p.mappingRequestValidators().stream()).collect(Collectors.toList()) ); @@ -1066,11 +1059,6 @@ public RestController getRestController() { return restController; } - // Visible for testing - UnaryOperator getRestWrapper() { - return restWrapper; - } - /** * The DynamicActionRegistry maintains a registry mapping {@link ActionType} instances to {@link TransportAction} instances. *

diff --git a/server/src/main/java/org/opensearch/identity/IdentityService.java b/server/src/main/java/org/opensearch/identity/IdentityService.java index 27596fb73963d..fb9a05ddb35c9 100644 --- a/server/src/main/java/org/opensearch/identity/IdentityService.java +++ b/server/src/main/java/org/opensearch/identity/IdentityService.java @@ -14,11 +14,9 @@ import org.opensearch.plugins.IdentityAwarePlugin; import org.opensearch.plugins.IdentityPlugin; import org.opensearch.plugins.Plugin; -import org.opensearch.rest.RestHandler; import org.opensearch.threadpool.ThreadPool; import java.util.List; -import java.util.function.UnaryOperator; import java.util.stream.Collectors; /** @@ -65,13 +63,6 @@ public TokenManager getTokenManager() { return identityPlugin.getTokenManager(); } - /** - * Gets the RestHandlerWrapper to authenticate the request - */ - public UnaryOperator authenticate() { - return identityPlugin.authenticate(this.threadPool.getThreadContext()); - } - public void initializeIdentityAwarePlugins(final List identityAwarePlugins) { if (identityAwarePlugins != null) { for (IdentityAwarePlugin plugin : identityAwarePlugins) { diff --git a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java index 0341361b64d2d..b40af14231fb9 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java @@ -9,15 +9,9 @@ package org.opensearch.plugins; import org.opensearch.common.annotation.ExperimentalApi; -import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.TokenManager; -import org.opensearch.rest.RestHandler; - -import java.util.function.UnaryOperator; - -import static org.opensearch.rest.RestController.PASS_THROUGH_REST_HANDLER_WRAPPER; /** * Plugin that provides identity and access control for OpenSearch @@ -40,15 +34,6 @@ public interface IdentityPlugin { */ TokenManager getTokenManager(); - /** - * Returns a function used to wrap each rest request before handling the request. - * The returned {@link UnaryOperator} is called for every incoming rest request and receives - * the original rest handler as it's input. - */ - default UnaryOperator authenticate(ThreadContext threadContext) { - return PASS_THROUGH_REST_HANDLER_WRAPPER; - } - /** * Gets a subject corresponding to the passed plugin that can be utilized to perform transport actions * in the plugin system context diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 0773d9d0fda03..eb1d22b726bd8 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -110,8 +110,6 @@ public class RestController implements HttpServerTransport.Dispatcher { private final PathTrie handlers = new PathTrie<>(RestUtils.REST_DECODER); - public static final UnaryOperator PASS_THROUGH_REST_HANDLER_WRAPPER = (h) -> h; - private final UnaryOperator handlerWrapper; private final NodeClient client; @@ -132,10 +130,10 @@ public RestController( this.headersToCopy = headersToCopy; this.usageService = usageService; if (handlerWrapper == null) { - this.handlerWrapper = PASS_THROUGH_REST_HANDLER_WRAPPER; - } else { - this.handlerWrapper = handlerWrapper; + handlerWrapper = h -> h; // passthrough if no wrapper set } + + this.handlerWrapper = handlerWrapper; this.client = client; this.circuitBreakerService = circuitBreakerService; registerHandlerNoWrap( diff --git a/server/src/test/java/org/opensearch/action/ActionModuleTests.java b/server/src/test/java/org/opensearch/action/ActionModuleTests.java index 8fddf84cb0597..8abe53fabb557 100644 --- a/server/src/test/java/org/opensearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/opensearch/action/ActionModuleTests.java @@ -49,13 +49,8 @@ import org.opensearch.core.action.ActionResponse; import org.opensearch.extensions.ExtensionsManager; import org.opensearch.identity.IdentityService; -import org.opensearch.identity.PluginSubject; -import org.opensearch.identity.Subject; -import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.ActionPlugin; import org.opensearch.plugins.ActionPlugin.ActionHandler; -import org.opensearch.plugins.IdentityPlugin; -import org.opensearch.plugins.Plugin; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestController; import org.opensearch.rest.RestHandler; @@ -74,15 +69,10 @@ import java.util.List; import java.util.Set; import java.util.function.Supplier; -import java.util.function.UnaryOperator; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; -import static org.opensearch.rest.RestController.PASS_THROUGH_REST_HANDLER_WRAPPER; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasEntry; -import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.startsWith; import static org.mockito.Mockito.mock; @@ -171,124 +161,6 @@ public List routes() { }) ); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); - assertThat(actionModule.getRestWrapper(), equalTo(PASS_THROUGH_REST_HANDLER_WRAPPER)); - } - - private class TestIdentityPlugin implements IdentityPlugin { - - @Override - public Subject getCurrentSubject() { - return null; - } - - @Override - public TokenManager getTokenManager() { - return null; - } - - @Override - public UnaryOperator authenticate(ThreadContext threadContext) { - return (h) -> { - threadContext.putHeader("test_header", "foo"); - return h; - }; - } - - @Override - public PluginSubject getPluginSubject(Plugin plugin) { - return null; - } - } - - private class TestActionPlugin implements ActionPlugin { - - private ThreadPool threadPool; - - public TestActionPlugin(ThreadPool threadPool) { - this.threadPool = threadPool; - } - - @Override - public UnaryOperator getRestHandlerWrapper(ThreadContext threadContext) { - return (h) -> { - threadContext.putHeader("test_header", "bar"); - return h; - }; - } - } - - public void testSetupRestHandlerWithIdentityPlugin() throws IOException { - ThreadPool threadPool = mock(ThreadPool.class); - IdentityPlugin testIdentityPlugin = new TestIdentityPlugin(); - SettingsModule settings = new SettingsModule(Settings.EMPTY); - UsageService usageService = new UsageService(); - IdentityService identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of(testIdentityPlugin)); - ActionModule actionModule = new ActionModule( - settings.getSettings(), - new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - settings.getIndexScopedSettings(), - settings.getClusterSettings(), - settings.getSettingsFilter(), - threadPool, - emptyList(), - null, - null, - usageService, - null, - identityService, - new ExtensionsManager(Set.of(), identityService) - ); - assertThat(actionModule.getRestWrapper(), not(equalTo(PASS_THROUGH_REST_HANDLER_WRAPPER))); - } - - public void testSetupRestHandlerWithActionPluginNoIdentityPlugin() throws IOException { - ThreadPool threadPool = mock(ThreadPool.class); - TestActionPlugin testActionPlugin = new TestActionPlugin(threadPool); - SettingsModule settings = new SettingsModule(Settings.EMPTY); - UsageService usageService = new UsageService(); - ActionModule actionModule = new ActionModule( - settings.getSettings(), - new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - settings.getIndexScopedSettings(), - settings.getClusterSettings(), - settings.getSettingsFilter(), - threadPool, - List.of(testActionPlugin), - null, - null, - usageService, - null, - new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()), - new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of())) - ); - assertThat(actionModule.getRestWrapper(), not(equalTo(PASS_THROUGH_REST_HANDLER_WRAPPER))); - } - - public void testSetupRestHandlerWithIdentityPluginOverrideActionPlugin() throws IOException { - IdentityPlugin testIdentityPlugin = new TestIdentityPlugin(); - ThreadPool threadPool = mock(ThreadPool.class); - TestActionPlugin testActionPlugin = new TestActionPlugin(threadPool); - SettingsModule settings = new SettingsModule(Settings.EMPTY); - UsageService usageService = new UsageService(); - IdentityService identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of(testIdentityPlugin)); - ActionModule actionModule = new ActionModule( - settings.getSettings(), - new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - settings.getIndexScopedSettings(), - settings.getClusterSettings(), - settings.getSettingsFilter(), - threadPool, - List.of(testActionPlugin), - null, - null, - usageService, - null, - identityService, - new ExtensionsManager(Set.of(), identityService) - ); - assertThat(actionModule.getRestWrapper(), not(equalTo(PASS_THROUGH_REST_HANDLER_WRAPPER))); - assertThat(actionModule.getRestWrapper().getClass().getName(), not(containsString("TestActionPlugin"))); - assertThat(actionModule.getRestWrapper().getClass().getName(), containsString("TestIdentityPlugin")); } public void testPluginCantOverwriteBuiltinRestHandler() throws IOException { From ca65e4bb50bf8fbb3fa97221fe05a767895a018a Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 29 Aug 2024 15:47:55 -0400 Subject: [PATCH 09/15] Move RestTokenExtractor to identity-shiro plugin Signed-off-by: Craig Perkins --- .../org/opensearch/identity/shiro}/RestTokenExtractor.java | 4 +++- .../org/opensearch/identity/shiro/ShiroIdentityPlugin.java | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) rename {server/src/main/java/org/opensearch/identity/tokens => plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro}/RestTokenExtractor.java (92%) diff --git a/server/src/main/java/org/opensearch/identity/tokens/RestTokenExtractor.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/RestTokenExtractor.java similarity index 92% rename from server/src/main/java/org/opensearch/identity/tokens/RestTokenExtractor.java rename to plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/RestTokenExtractor.java index 4bd3ebdded588..b7318147d2b17 100644 --- a/server/src/main/java/org/opensearch/identity/tokens/RestTokenExtractor.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/RestTokenExtractor.java @@ -5,11 +5,13 @@ * this file be licensed under the Apache-2.0 license or a * compatible open source license. */ -package org.opensearch.identity.tokens; +package org.opensearch.identity.shiro; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.core.common.Strings; +import org.opensearch.identity.tokens.AuthToken; +import org.opensearch.identity.tokens.BasicAuthToken; import org.opensearch.rest.RestRequest; import java.util.Collections; 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 f4700af981761..46f180aa02a4a 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 @@ -27,7 +27,6 @@ import org.opensearch.identity.PluginSubject; import org.opensearch.identity.Subject; import org.opensearch.identity.tokens.AuthToken; -import org.opensearch.identity.tokens.RestTokenExtractor; import org.opensearch.identity.tokens.TokenManager; import org.opensearch.plugins.ActionPlugin; import org.opensearch.plugins.IdentityPlugin; From c286da66191ca09385854e3823a05a242dde2f61 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 29 Aug 2024 15:55:03 -0400 Subject: [PATCH 10/15] Remove change in IdentityService Signed-off-by: Craig Perkins --- .../src/main/java/org/opensearch/identity/IdentityService.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/identity/IdentityService.java b/server/src/main/java/org/opensearch/identity/IdentityService.java index fb9a05ddb35c9..03f937180f4ba 100644 --- a/server/src/main/java/org/opensearch/identity/IdentityService.java +++ b/server/src/main/java/org/opensearch/identity/IdentityService.java @@ -29,11 +29,9 @@ public class IdentityService { private final Settings settings; private final IdentityPlugin identityPlugin; - private final ThreadPool threadPool; public IdentityService(final Settings settings, final ThreadPool threadPool, final List identityPlugins) { this.settings = settings; - this.threadPool = threadPool; if (identityPlugins.size() == 0) { log.debug("Identity plugins size is 0"); From c11aed4f0b0b293f23dce4abfe69877f79d2c540 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 29 Aug 2024 15:56:49 -0400 Subject: [PATCH 11/15] Remove changes in ActionModuleTests Signed-off-by: Craig Perkins --- .../test/java/org/opensearch/action/ActionModuleTests.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/opensearch/action/ActionModuleTests.java b/server/src/test/java/org/opensearch/action/ActionModuleTests.java index 8abe53fabb557..6b8951dd43d11 100644 --- a/server/src/test/java/org/opensearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/opensearch/action/ActionModuleTests.java @@ -187,7 +187,6 @@ public String getName() { }; SettingsModule settings = new SettingsModule(Settings.EMPTY); ThreadPool threadPool = new TestThreadPool(getTestName()); - IdentityService identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()); try { UsageService usageService = new UsageService(); ActionModule actionModule = new ActionModule( @@ -202,7 +201,7 @@ public String getName() { null, usageService, null, - identityService, + null, null ); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); @@ -239,7 +238,6 @@ public List getRestHandlers( SettingsModule settings = new SettingsModule(Settings.EMPTY); ThreadPool threadPool = new TestThreadPool(getTestName()); - IdentityService identityService = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()); try { UsageService usageService = new UsageService(); ActionModule actionModule = new ActionModule( @@ -254,7 +252,7 @@ public List getRestHandlers( null, usageService, null, - identityService, + null, null ); actionModule.initRestHandlers(null); From d9223a976db20ccdc406bd6cc56c469af815e0a4 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 29 Aug 2024 17:10:11 -0400 Subject: [PATCH 12/15] Add tests for RestTokenExtractor Signed-off-by: Craig Perkins --- .../shiro/RestTokenExtractorTests.java | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/RestTokenExtractorTests.java diff --git a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/RestTokenExtractorTests.java b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/RestTokenExtractorTests.java new file mode 100644 index 0000000000000..27f3706895ee6 --- /dev/null +++ b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/RestTokenExtractorTests.java @@ -0,0 +1,45 @@ +/* + * 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.identity.tokens.AuthToken; +import org.opensearch.identity.tokens.BasicAuthToken; +import org.opensearch.rest.RestRequest; +import org.opensearch.test.OpenSearchTestCase; +import org.opensearch.test.rest.FakeRestRequest; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.List; +import java.util.Map; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; + +public class RestTokenExtractorTests extends OpenSearchTestCase { + + public void testAuthorizationHeaderExtractionWithBasicAuthToken() { + String basicAuthHeader = Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8)); + RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders( + Map.of(RestTokenExtractor.AUTH_HEADER_NAME, List.of(BasicAuthToken.TOKEN_IDENTIFIER + " " + basicAuthHeader)) + ).build(); + AuthToken extractedToken = RestTokenExtractor.extractToken(fakeRequest); + assertThat(extractedToken, instanceOf(BasicAuthToken.class)); + assertThat(extractedToken.asAuthHeaderValue(), equalTo(basicAuthHeader)); + } + + public void testAuthorizationHeaderExtractionWithUnknownToken() { + String authHeader = "foo"; + RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders( + Map.of(RestTokenExtractor.AUTH_HEADER_NAME, List.of(authHeader)) + ).build(); + AuthToken extractedToken = RestTokenExtractor.extractToken(fakeRequest); + assertNull(extractedToken); + } +} From df4c0423fb3fbd4424332395baa32eb0520b52aa Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 11 Sep 2024 18:30:28 -0400 Subject: [PATCH 13/15] Remove DelegatingRestHandler Signed-off-by: Craig Perkins --- .../identity/shiro/DelegatingRestHandler.java | 80 ------------------- .../identity/shiro/ShiroIdentityPlugin.java | 5 +- .../shiro/DelegatingRestHandlerTests.java | 65 --------------- 3 files changed, 4 insertions(+), 146 deletions(-) delete mode 100644 plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java delete mode 100644 plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java deleted file mode 100644 index 2108b42e258ab..0000000000000 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java +++ /dev/null @@ -1,80 +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.client.node.NodeClient; -import org.opensearch.rest.RestChannel; -import org.opensearch.rest.RestHandler; -import org.opensearch.rest.RestRequest; - -import java.util.List; -import java.util.Objects; - -/** - * Delegating RestHandler that delegates all implementations to original handler - */ -public class DelegatingRestHandler implements RestHandler { - - protected final RestHandler delegate; - - public DelegatingRestHandler(RestHandler delegate) { - Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); - this.delegate = delegate; - } - - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - delegate.handleRequest(request, channel, client); - } - - @Override - public boolean canTripCircuitBreaker() { - return delegate.canTripCircuitBreaker(); - } - - @Override - public boolean supportsContentStream() { - return delegate.supportsContentStream(); - } - - @Override - public boolean allowsUnsafeBuffers() { - return delegate.allowsUnsafeBuffers(); - } - - @Override - public List routes() { - return delegate.routes(); - } - - @Override - public List deprecatedRoutes() { - return delegate.deprecatedRoutes(); - } - - @Override - public List replacedRoutes() { - return delegate.replacedRoutes(); - } - - @Override - public boolean allowSystemIndexAccessByDefault() { - return delegate.allowSystemIndexAccessByDefault(); - } - - @Override - public String toString() { - return delegate.toString(); - } - - @Override - public boolean supportsStreaming() { - return delegate.supportsStreaming(); - } -} 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 46f180aa02a4a..7ea607e6ed6e2 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 @@ -113,9 +113,12 @@ public UnaryOperator getRestHandlerWrapper(ThreadContext threadCont return AuthcRestHandler::new; } - class AuthcRestHandler extends DelegatingRestHandler { + class AuthcRestHandler extends RestHandler.Wrapper { + private final RestHandler delegate; + public AuthcRestHandler(RestHandler original) { super(original); + this.delegate = original; } @Override diff --git a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java deleted file mode 100644 index b376e68622e72..0000000000000 --- a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/DelegatingRestHandlerTests.java +++ /dev/null @@ -1,65 +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.client.node.NodeClient; -import org.opensearch.core.common.bytes.BytesArray; -import org.opensearch.core.rest.RestStatus; -import org.opensearch.rest.BytesRestResponse; -import org.opensearch.rest.RestChannel; -import org.opensearch.rest.RestHandler; -import org.opensearch.rest.RestRequest; -import org.opensearch.test.OpenSearchTestCase; - -import java.lang.reflect.Method; -import java.lang.reflect.Modifier; -import java.util.Arrays; -import java.util.List; -import java.util.stream.Collectors; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; - -public class DelegatingRestHandlerTests extends OpenSearchTestCase { - - public void testDelegatingRestHandlerShouldActAsOriginal() throws Exception { - RestHandler rh = new RestHandler() { - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); - } - }; - RestHandler handlerSpy = spy(rh); - DelegatingRestHandler drh = new DelegatingRestHandler(handlerSpy); - - List overridableMethods = Arrays.stream(RestHandler.class.getMethods()) - .filter( - m -> !(Modifier.isPrivate(m.getModifiers()) || Modifier.isStatic(m.getModifiers()) || Modifier.isFinal(m.getModifiers())) - ) - .collect(Collectors.toList()); - - for (Method method : overridableMethods) { - int argCount = method.getParameterCount(); - Object[] args = new Object[argCount]; - for (int i = 0; i < argCount; i++) { - args[i] = any(); - } - if (args.length > 0) { - method.invoke(drh, args); - } else { - method.invoke(drh); - } - method.invoke(verify(handlerSpy, times(1)), args); - } - verifyNoMoreInteractions(handlerSpy); - } -} From d3bcc1cedd17727a7b8f879d88e21e117393a131 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 12 Sep 2024 09:53:36 -0400 Subject: [PATCH 14/15] Call super instead of keeping a reference to the delegated restHandler Signed-off-by: Craig Perkins --- .../org/opensearch/identity/shiro/ShiroIdentityPlugin.java | 6 ++---- 1 file 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 7ea607e6ed6e2..74b1149deadd4 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 @@ -114,11 +114,9 @@ public UnaryOperator getRestHandlerWrapper(ThreadContext threadCont } class AuthcRestHandler extends RestHandler.Wrapper { - private final RestHandler delegate; public AuthcRestHandler(RestHandler original) { super(original); - this.delegate = original; } @Override @@ -128,13 +126,13 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c // If no token was found, continue executing the request if (token == null) { // Authentication did not fail so return true. Authorization is handled at the action level. - delegate.handleRequest(request, channel, client); + super.handleRequest(request, channel, client); return; } ShiroSubject shiroSubject = (ShiroSubject) getCurrentSubject(); shiroSubject.authenticate(token); // Caller was authorized, forward the request to the handler - delegate.handleRequest(request, channel, client); + super.handleRequest(request, channel, client); } catch (final Exception e) { final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, e.getMessage()); channel.sendResponse(bytesRestResponse); From c8489fea8e4e74954db4fd6d7a270494c1c475ba Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 12 Sep 2024 11:24:56 -0400 Subject: [PATCH 15/15] Address code review comments Signed-off-by: Craig Perkins --- .../opensearch/identity/shiro/ShiroIdentityPlugin.java | 4 +--- ...estTokenExtractor.java => ShiroTokenExtractor.java} | 4 ++-- ...tractorTests.java => ShiroTokenExtractorTests.java} | 10 +++++----- 3 files changed, 8 insertions(+), 10 deletions(-) rename plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/{RestTokenExtractor.java => ShiroTokenExtractor.java} (93%) rename plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/{RestTokenExtractorTests.java => ShiroTokenExtractorTests.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 74b1149deadd4..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 @@ -16,7 +16,6 @@ import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.core.common.io.stream.NamedWriteableRegistry; @@ -48,7 +47,6 @@ /** * Identity implementation with Shiro */ -@ExperimentalApi public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin, ActionPlugin { private Logger log = LogManager.getLogger(this.getClass()); @@ -122,7 +120,7 @@ public AuthcRestHandler(RestHandler original) { @Override public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { try { - final AuthToken token = RestTokenExtractor.extractToken(request); + final AuthToken token = ShiroTokenExtractor.extractToken(request); // If no token was found, continue executing the request if (token == null) { // Authentication did not fail so return true. Authorization is handled at the action level. diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/RestTokenExtractor.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenExtractor.java similarity index 93% rename from plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/RestTokenExtractor.java rename to plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenExtractor.java index b7318147d2b17..86be5ca664daa 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/RestTokenExtractor.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenExtractor.java @@ -20,9 +20,9 @@ /** * Extracts tokens from RestRequests used for authentication */ -public class RestTokenExtractor { +public class ShiroTokenExtractor { - private static final Logger logger = LogManager.getLogger(RestTokenExtractor.class); + private static final Logger logger = LogManager.getLogger(ShiroTokenExtractor.class); public final static String AUTH_HEADER_NAME = "Authorization"; diff --git a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/RestTokenExtractorTests.java b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroTokenExtractorTests.java similarity index 76% rename from plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/RestTokenExtractorTests.java rename to plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroTokenExtractorTests.java index 27f3706895ee6..4dc398bacb707 100644 --- a/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/RestTokenExtractorTests.java +++ b/plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroTokenExtractorTests.java @@ -22,14 +22,14 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; -public class RestTokenExtractorTests extends OpenSearchTestCase { +public class ShiroTokenExtractorTests extends OpenSearchTestCase { public void testAuthorizationHeaderExtractionWithBasicAuthToken() { String basicAuthHeader = Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8)); RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders( - Map.of(RestTokenExtractor.AUTH_HEADER_NAME, List.of(BasicAuthToken.TOKEN_IDENTIFIER + " " + basicAuthHeader)) + Map.of(ShiroTokenExtractor.AUTH_HEADER_NAME, List.of(BasicAuthToken.TOKEN_IDENTIFIER + " " + basicAuthHeader)) ).build(); - AuthToken extractedToken = RestTokenExtractor.extractToken(fakeRequest); + AuthToken extractedToken = ShiroTokenExtractor.extractToken(fakeRequest); assertThat(extractedToken, instanceOf(BasicAuthToken.class)); assertThat(extractedToken.asAuthHeaderValue(), equalTo(basicAuthHeader)); } @@ -37,9 +37,9 @@ public void testAuthorizationHeaderExtractionWithBasicAuthToken() { public void testAuthorizationHeaderExtractionWithUnknownToken() { String authHeader = "foo"; RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders( - Map.of(RestTokenExtractor.AUTH_HEADER_NAME, List.of(authHeader)) + Map.of(ShiroTokenExtractor.AUTH_HEADER_NAME, List.of(authHeader)) ).build(); - AuthToken extractedToken = RestTokenExtractor.extractToken(fakeRequest); + AuthToken extractedToken = ShiroTokenExtractor.extractToken(fakeRequest); assertNull(extractedToken); } }