Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove identity-related feature flagged code from the RestController #15430

Merged
merged 25 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e6b82ba
Add authenticate to IdentityPlugin interface
cwperks Aug 26, 2024
bc610a2
Handle null
cwperks Aug 27, 2024
1abfe97
Fix tests
cwperks Aug 27, 2024
cf3f41f
Merge branch 'main' into identity-plugin-authenticate
cwperks Aug 28, 2024
cdf1892
Fix ActionModuleTests
cwperks Aug 28, 2024
09e8fc0
Add to CHANGELOG
cwperks Aug 28, 2024
4ebccdd
Merge branch 'main' into identity-plugin-authenticate
cwperks Aug 29, 2024
97dd935
Add DelegatingRestHandlerTests
cwperks Aug 29, 2024
7481868
Address forbiddenApi
cwperks Aug 29, 2024
7da9bd7
Remove authenticate from IdentityPlugin and keep RestController featu…
cwperks Aug 29, 2024
ca65e4b
Move RestTokenExtractor to identity-shiro plugin
cwperks Aug 29, 2024
c286da6
Remove change in IdentityService
cwperks Aug 29, 2024
c11aed4
Remove changes in ActionModuleTests
cwperks Aug 29, 2024
d9223a9
Add tests for RestTokenExtractor
cwperks Aug 29, 2024
aa047f9
Merge branch 'main' into identity-plugin-authenticate
cwperks Aug 30, 2024
c107601
Merge branch 'main' into identity-plugin-authenticate
cwperks Sep 3, 2024
05dd6ac
Merge branch 'main' into identity-plugin-authenticate
cwperks Sep 4, 2024
b0ce4a8
Merge branch 'main' into identity-plugin-authenticate
cwperks Sep 10, 2024
6409e73
Merge branch 'main' into identity-plugin-authenticate
cwperks Sep 11, 2024
df4c042
Remove DelegatingRestHandler
cwperks Sep 11, 2024
d3bcc1c
Call super instead of keeping a reference to the delegated restHandler
cwperks Sep 12, 2024
2a56781
Merge branch 'main' into identity-plugin-authenticate
cwperks Sep 12, 2024
c8489fe
Address code review comments
cwperks Sep 12, 2024
34bc922
Merge branch 'main' into identity-plugin-authenticate
cwperks Sep 18, 2024
c2d9a3a
Merge branch 'main' into identity-plugin-authenticate
cwperks Sep 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +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))
- 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))
Expand Down
Original file line number Diff line number Diff line change
@@ -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) {
cwperks marked this conversation as resolved.
Show resolved Hide resolved
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<Route> routes() {
return delegate.routes();
}

@Override
public List<DeprecatedRoute> deprecatedRoutes() {
return delegate.deprecatedRoutes();
}

@Override
public List<ReplacedRoute> replacedRoutes() {
return delegate.replacedRoutes();
}

@Override
public boolean allowSystemIndexAccessByDefault() {
return delegate.allowSystemIndexAccessByDefault();
}

@Override
public String toString() {
return delegate.toString();

Check warning on line 73 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java

View check run for this annotation

Codecov / codecov/patch

plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/DelegatingRestHandler.java#L73

Added line #L73 was not covered by tests
}

@Override
public boolean supportsStreaming() {
return delegate.supportsStreaming();
}
}
cwperks marked this conversation as resolved.
Show resolved Hide resolved
cwperks marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,43 @@
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
cwperks marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down Expand Up @@ -101,6 +109,36 @@
}

@Override
public UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
return AuthcRestHandler::new;

Check warning on line 113 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java

View check run for this annotation

Codecov / codecov/patch

plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java#L113

Added line #L113 was not covered by tests
}

class AuthcRestHandler extends DelegatingRestHandler {
public AuthcRestHandler(RestHandler original) {
cwperks marked this conversation as resolved.
Show resolved Hide resolved
super(original);
}

Check warning on line 119 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java

View check run for this annotation

Codecov / codecov/patch

plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java#L117-L119

Added lines #L117 - L119 were not covered by tests

@Override
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
cwperks marked this conversation as resolved.
Show resolved Hide resolved
try {
final AuthToken token = RestTokenExtractor.extractToken(request);

Check warning on line 124 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java

View check run for this annotation

Codecov / codecov/patch

plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java#L124

Added line #L124 was not covered by tests
// 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);
cwperks marked this conversation as resolved.
Show resolved Hide resolved
return;

Check warning on line 129 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java

View check run for this annotation

Codecov / codecov/patch

plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java#L128-L129

Added lines #L128 - L129 were not covered by tests
}
ShiroSubject shiroSubject = (ShiroSubject) getCurrentSubject();
shiroSubject.authenticate(token);

Check warning on line 132 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java

View check run for this annotation

Codecov / codecov/patch

plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java#L131-L132

Added lines #L131 - L132 were not covered by tests
// Caller was authorized, forward the request to the handler
delegate.handleRequest(request, channel, client);
cwperks marked this conversation as resolved.
Show resolved Hide resolved
} catch (final Exception e) {
final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, e.getMessage());
channel.sendResponse(bytesRestResponse);
}
}

Check warning on line 139 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java

View check run for this annotation

Codecov / codecov/patch

plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java#L134-L139

Added lines #L134 - L139 were not covered by tests
}

public PluginSubject getPluginSubject(Plugin plugin) {
return new ShiroPluginSubject(threadPool);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@

/**
* OpenSearch specific security manager implementation
*
* @opensearch.experimental
*/
public class ShiroSecurityManager extends DefaultSecurityManager {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@

/**
* Subject backed by Shiro
*
* @opensearch.experimental
*/
public class ShiroSubject implements UserSubject {
private final ShiroTokenManager authTokenHandler;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@

/**
* Extracts Shiro's {@link AuthenticationToken} from different types of auth headers
*
* @opensearch.experimental
*/
class ShiroTokenManager implements TokenManager {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

/**
* Password matcher for BCrypt
*
* @opensearch.experimental
*/
public class BCryptPasswordMatcher implements CredentialsMatcher {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@

/**
* Internal Realm is a custom realm using the internal OpenSearch IdP
*
* @opensearch.experimental
cwperks marked this conversation as resolved.
Show resolved Hide resolved
*/
public class OpenSearchRealm extends AuthenticatingRealm {
private static final String DEFAULT_REALM_NAME = "internal";
Expand Down Expand Up @@ -93,7 +91,7 @@
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");

Check warning on line 94 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java

View check run for this annotation

Codecov / codecov/patch

plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/OpenSearchRealm.java#L94

Added line #L94 was not covered by tests
}
return userRecord;
}
Expand Down Expand Up @@ -131,7 +129,7 @@
return sai;
} else {
// Bad password
throw new IncorrectCredentialsException();
throw new IncorrectCredentialsException("Incorrect credentials");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@

/**
* A non-volatile and immutable object in the storage.
*
* @opensearch.experimental
*/
public class User {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* 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 {
cwperks marked this conversation as resolved.
Show resolved Hide resolved
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<Method> 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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,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() {
Expand Down
Loading
Loading