Remove identity-related feature flagged code from the RestController (#…

…15430) * Add authenticate to IdentityPlugin interface Signed-off-by: Craig Perkins <[email protected]> * Handle null Signed-off-by: Craig Perkins <[email protected]> * Fix tests Signed-off-by: Craig Perkins <[email protected]> * Fix ActionModuleTests Signed-off-by: Craig Perkins <[email protected]> * Add to CHANGELOG Signed-off-by: Craig Perkins <[email protected]> * Add DelegatingRestHandlerTests Signed-off-by: Craig Perkins <[email protected]> * Address forbiddenApi Signed-off-by: Craig Perkins <[email protected]> * Remove authenticate from IdentityPlugin and keep RestController feature flagged code removed Signed-off-by: Craig Perkins <[email protected]> * Move RestTokenExtractor to identity-shiro plugin Signed-off-by: Craig Perkins <[email protected]> * Remove change in IdentityService Signed-off-by: Craig Perkins <[email protected]> * Remove changes in ActionModuleTests Signed-off-by: Craig Perkins <[email protected]> * Add tests for RestTokenExtractor Signed-off-by: Craig Perkins <[email protected]> * Remove DelegatingRestHandler Signed-off-by: Craig Perkins <[email protected]> * Call super instead of keeping a reference to the delegated restHandler Signed-off-by: Craig Perkins <[email protected]> * Address code review comments Signed-off-by: Craig Perkins <[email protected]> --------- Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Craig Perkins <[email protected]>
opensearch-project · Sep 19, 2024 · 1bc81d3 · 1bc81d3
1 parent 77ddfd6
commit 1bc81d3
Show file tree

Hide file tree

Showing 18 changed files with 118 additions and 143 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [Workload Management] QueryGroup resource cancellation framework changes ([#15651](https://github.com/opensearch-project/OpenSearch/pull/15651))
 - Fallback to Remote cluster-state on Term-Version check mismatch - ([#15424](https://github.com/opensearch-project/OpenSearch/pull/15424))
 - Implement WithFieldName interface in ValuesSourceAggregationBuilder & FieldSortBuilder ([#15916](https://github.com/opensearch-project/OpenSearch/pull/15916))
+- Remove identity-related feature flagged code from the RestController ([#15430](https://github.com/opensearch-project/OpenSearch/pull/15430))
 
 ### Dependencies
 - Bump `com.azure:azure-identity` from 1.13.0 to 1.13.2 ([#15578](https://github.com/opensearch-project/OpenSearch/pull/15578))

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
@@ -13,35 +13,41 @@
 import org.apache.shiro.SecurityUtils;
 import org.apache.shiro.mgt.SecurityManager;
 import org.opensearch.client.Client;
+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;
+import org.opensearch.core.rest.RestStatus;
 import org.opensearch.core.xcontent.NamedXContentRegistry;
 import org.opensearch.env.Environment;
 import org.opensearch.env.NodeEnvironment;
 import org.opensearch.identity.PluginSubject;
 import org.opensearch.identity.Subject;
+import org.opensearch.identity.tokens.AuthToken;
 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;
+import org.opensearch.rest.BytesRestResponse;
+import org.opensearch.rest.RestChannel;
+import org.opensearch.rest.RestHandler;
+import org.opensearch.rest.RestRequest;
 import org.opensearch.script.ScriptService;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.watcher.ResourceWatcherService;
 
 import java.util.Collection;
 import java.util.Collections;
 import java.util.function.Supplier;
+import java.util.function.UnaryOperator;
 
 /**
  * Identity implementation with Shiro
- *
- * @opensearch.experimental
  */
-@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;
@@ -101,6 +107,37 @@ public TokenManager getTokenManager() {
     }
 
     @Override
+    public UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
+        return AuthcRestHandler::new;
+    }
+
+    class AuthcRestHandler extends RestHandler.Wrapper {
+
+        public AuthcRestHandler(RestHandler original) {
+            super(original);
+        }
+
+        @Override
+        public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
+            try {
+                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.
+                    super.handleRequest(request, channel, client);
+                    return;
+                }
+                ShiroSubject shiroSubject = (ShiroSubject) getCurrentSubject();
+                shiroSubject.authenticate(token);
+                // Caller was authorized, forward the request to the handler
+                super.handleRequest(request, channel, client);
+            } catch (final Exception e) {
+                final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, e.getMessage());
+                channel.sendResponse(bytesRestResponse);
+            }
+        }
+    }
+
     public PluginSubject getPluginSubject(Plugin plugin) {
         return new ShiroPluginSubject(threadPool);
     }

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
@@ -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
@@ -17,8 +17,6 @@
 
 /**
  * Subject backed by Shiro
- *
- * @opensearch.experimental
  */
 public class ShiroSubject implements UserSubject {
     private final ShiroTokenManager authTokenHandler;

diff --git a/...h/identity/tokens/RestTokenExtractor.java → ...h/identity/shiro/ShiroTokenExtractor.java b/...h/identity/tokens/RestTokenExtractor.java → ...h/identity/shiro/ShiroTokenExtractor.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;
@@ -18,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/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/...entity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java b/...entity-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/...ins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java b/...ins/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";
@@ -93,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;
     }
@@ -131,7 +129,7 @@ protected AuthenticationInfo doGetAuthenticationInfo(final AuthenticationToken t
                 return sai;
             } else {
                 // Bad password
-                throw new IncorrectCredentialsException();
+                throw new IncorrectCredentialsException("Incorrect credentials");
             }
         }
 

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
@@ -12,8 +12,6 @@
 
 /**
  * A non-volatile and immutable object in the storage.
- *
- * @opensearch.experimental
  */
 public class User {
 

diff --git a/.../identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java b/.../identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java
@@ -13,35 +13,35 @@
 import org.opensearch.identity.IdentityService;
 import org.opensearch.plugins.IdentityPlugin;
 import org.opensearch.test.OpenSearchTestCase;
-import org.opensearch.threadpool.TestThreadPool;
+import org.opensearch.threadpool.ThreadPool;
 
 import java.util.List;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 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() {
-        TestThreadPool threadPool = new TestThreadPool(getTestName());
         IdentityPlugin identityPlugin1 = new ShiroIdentityPlugin(Settings.EMPTY);
         List<IdentityPlugin> pluginList1 = List.of(identityPlugin1);
-        IdentityService identityService1 = new IdentityService(Settings.EMPTY, threadPool, pluginList1);
+        IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList1);
         assertThat(identityService1.getTokenManager(), is(instanceOf(ShiroTokenManager.class)));
-        terminate(threadPool);
     }
 
     public void testMultipleIdentityPluginsFail() {
-        TestThreadPool threadPool = new TestThreadPool(getTestName());
         IdentityPlugin identityPlugin1 = new ShiroIdentityPlugin(Settings.EMPTY);
         IdentityPlugin identityPlugin2 = new ShiroIdentityPlugin(Settings.EMPTY);
         IdentityPlugin identityPlugin3 = new ShiroIdentityPlugin(Settings.EMPTY);
         List<IdentityPlugin> pluginList = List.of(identityPlugin1, identityPlugin2, identityPlugin3);
-        Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, threadPool, pluginList));
+        Exception ex = assertThrows(
+            OpenSearchException.class,
+            () -> new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList)
+        );
         assert (ex.getMessage().contains("Multiple identity plugins are not supported,"));
-        terminate(threadPool);
     }
 
 }
diff --git a/.../identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroTokenExtractorTests.java b/.../identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroTokenExtractorTests.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 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(ShiroTokenExtractor.AUTH_HEADER_NAME, List.of(BasicAuthToken.TOKEN_IDENTIFIER + " " + basicAuthHeader))
+        ).build();
+        AuthToken extractedToken = ShiroTokenExtractor.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(ShiroTokenExtractor.AUTH_HEADER_NAME, List.of(authHeader))
+        ).build();
+        AuthToken extractedToken = ShiroTokenExtractor.extractToken(fakeRequest);
+        assertNull(extractedToken);
+    }
+}
diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java
@@ -579,7 +579,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<String, ActionHandler<?, ?>> getActions() {

diff --git 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,11 +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.UserSubject;
-import org.opensearch.identity.tokens.AuthToken;
-import org.opensearch.identity.tokens.RestTokenExtractor;
 import org.opensearch.usage.UsageService;
 
 import java.io.ByteArrayOutputStream;
@@ -125,25 +119,23 @@ public class RestController implements HttpServerTransport.Dispatcher {
     /** Rest headers that are copied to internal requests made during a rest request. */
     private final Set<RestHeaderDefinition> headersToCopy;
     private final UsageService usageService;
-    private final IdentityService identityService;
 
     public RestController(
         Set<RestHeaderDefinition> headersToCopy,
         UnaryOperator<RestHandler> 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",
@@ -472,11 +464,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;
                 }
@@ -587,43 +574,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.getCurrentSubject();
-            if (currentSubject instanceof UserSubject) {
-                ((UserSubject) 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.
      */