Skip to content

Commit 1bc81d3

Browse files
cwperkscwperks
authored
Remove identity-related feature flagged code from the RestController (opensearch-project#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]>
1 parent 77ddfd6 commit 1bc81d3

File tree

18 files changed

+118
-143
lines changed

18 files changed

+118
-143
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
99
- [Workload Management] QueryGroup resource cancellation framework changes ([#15651](https://github.com/opensearch-project/OpenSearch/pull/15651))
1010
- Fallback to Remote cluster-state on Term-Version check mismatch - ([#15424](https://github.com/opensearch-project/OpenSearch/pull/15424))
1111
- Implement WithFieldName interface in ValuesSourceAggregationBuilder & FieldSortBuilder ([#15916](https://github.com/opensearch-project/OpenSearch/pull/15916))
12+
- Remove identity-related feature flagged code from the RestController ([#15430](https://github.com/opensearch-project/OpenSearch/pull/15430))
1213

1314
### Dependencies
1415
- Bump `com.azure:azure-identity` from 1.13.0 to 1.13.2 ([#15578](https://github.com/opensearch-project/OpenSearch/pull/15578))

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

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,35 +13,41 @@
1313
import org.apache.shiro.SecurityUtils;
1414
import org.apache.shiro.mgt.SecurityManager;
1515
import org.opensearch.client.Client;
16+
import org.opensearch.client.node.NodeClient;
1617
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
1718
import org.opensearch.cluster.service.ClusterService;
18-
import org.opensearch.common.annotation.ExperimentalApi;
1919
import org.opensearch.common.settings.Settings;
20+
import org.opensearch.common.util.concurrent.ThreadContext;
2021
import org.opensearch.core.common.io.stream.NamedWriteableRegistry;
22+
import org.opensearch.core.rest.RestStatus;
2123
import org.opensearch.core.xcontent.NamedXContentRegistry;
2224
import org.opensearch.env.Environment;
2325
import org.opensearch.env.NodeEnvironment;
2426
import org.opensearch.identity.PluginSubject;
2527
import org.opensearch.identity.Subject;
28+
import org.opensearch.identity.tokens.AuthToken;
2629
import org.opensearch.identity.tokens.TokenManager;
30+
import org.opensearch.plugins.ActionPlugin;
2731
import org.opensearch.plugins.IdentityPlugin;
2832
import org.opensearch.plugins.Plugin;
2933
import org.opensearch.repositories.RepositoriesService;
34+
import org.opensearch.rest.BytesRestResponse;
35+
import org.opensearch.rest.RestChannel;
36+
import org.opensearch.rest.RestHandler;
37+
import org.opensearch.rest.RestRequest;
3038
import org.opensearch.script.ScriptService;
3139
import org.opensearch.threadpool.ThreadPool;
3240
import org.opensearch.watcher.ResourceWatcherService;
3341

3442
import java.util.Collection;
3543
import java.util.Collections;
3644
import java.util.function.Supplier;
45+
import java.util.function.UnaryOperator;
3746

3847
/**
3948
* Identity implementation with Shiro
40-
*
41-
* @opensearch.experimental
4249
*/
43-
@ExperimentalApi
44-
public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin {
50+
public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin, ActionPlugin {
4551
private Logger log = LogManager.getLogger(this.getClass());
4652

4753
private final Settings settings;
@@ -101,6 +107,37 @@ public TokenManager getTokenManager() {
101107
}
102108

103109
@Override
110+
public UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
111+
return AuthcRestHandler::new;
112+
}
113+
114+
class AuthcRestHandler extends RestHandler.Wrapper {
115+
116+
public AuthcRestHandler(RestHandler original) {
117+
super(original);
118+
}
119+
120+
@Override
121+
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
122+
try {
123+
final AuthToken token = ShiroTokenExtractor.extractToken(request);
124+
// If no token was found, continue executing the request
125+
if (token == null) {
126+
// Authentication did not fail so return true. Authorization is handled at the action level.
127+
super.handleRequest(request, channel, client);
128+
return;
129+
}
130+
ShiroSubject shiroSubject = (ShiroSubject) getCurrentSubject();
131+
shiroSubject.authenticate(token);
132+
// Caller was authorized, forward the request to the handler
133+
super.handleRequest(request, channel, client);
134+
} catch (final Exception e) {
135+
final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, e.getMessage());
136+
channel.sendResponse(bytesRestResponse);
137+
}
138+
}
139+
}
140+
104141
public PluginSubject getPluginSubject(Plugin plugin) {
105142
return new ShiroPluginSubject(threadPool);
106143
}

‎plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSecurityManager.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515

1616
/**
1717
* OpenSearch specific security manager implementation
18-
*
19-
* @opensearch.experimental
2018
*/
2119
public class ShiroSecurityManager extends DefaultSecurityManager {
2220

‎plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@
1717

1818
/**
1919
* Subject backed by Shiro
20-
*
21-
* @opensearch.experimental
2220
*/
2321
public class ShiroSubject implements UserSubject {
2422
private final ShiroTokenManager authTokenHandler;

‎server/src/main/java/org/opensearch/identity/tokens/RestTokenExtractor.java renamed to ‎plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenExtractor.java

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@
55
* this file be licensed under the Apache-2.0 license or a
66
* compatible open source license.
77
*/
8-
package org.opensearch.identity.tokens;
8+
package org.opensearch.identity.shiro;
99

1010
import org.apache.logging.log4j.LogManager;
1111
import org.apache.logging.log4j.Logger;
1212
import org.opensearch.core.common.Strings;
13+
import org.opensearch.identity.tokens.AuthToken;
14+
import org.opensearch.identity.tokens.BasicAuthToken;
1315
import org.opensearch.rest.RestRequest;
1416

1517
import java.util.Collections;
@@ -18,9 +20,9 @@
1820
/**
1921
* Extracts tokens from RestRequests used for authentication
2022
*/
21-
public class RestTokenExtractor {
23+
public class ShiroTokenExtractor {
2224

23-
private static final Logger logger = LogManager.getLogger(RestTokenExtractor.class);
25+
private static final Logger logger = LogManager.getLogger(ShiroTokenExtractor.class);
2426

2527
public final static String AUTH_HEADER_NAME = "Authorization";
2628

‎plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroTokenManager.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,6 @@
3636

3737
/**
3838
* Extracts Shiro's {@link AuthenticationToken} from different types of auth headers
39-
*
40-
* @opensearch.experimental
4139
*/
4240
class ShiroTokenManager implements TokenManager {
4341

‎plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@
1616

1717
/**
1818
* Password matcher for BCrypt
19-
*
20-
* @opensearch.experimental
2119
*/
2220
public class BCryptPasswordMatcher implements CredentialsMatcher {
2321

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@
2525

2626
/**
2727
* Internal Realm is a custom realm using the internal OpenSearch IdP
28-
*
29-
* @opensearch.experimental
3028
*/
3129
public class OpenSearchRealm extends AuthenticatingRealm {
3230
private static final String DEFAULT_REALM_NAME = "internal";
@@ -93,7 +91,7 @@ public OpenSearchRealm build() {
9391
public User getInternalUser(final String principalIdentifier) throws UnknownAccountException {
9492
final User userRecord = internalUsers.get(principalIdentifier);
9593
if (userRecord == null) {
96-
throw new UnknownAccountException();
94+
throw new UnknownAccountException("Incorrect credentials");
9795
}
9896
return userRecord;
9997
}
@@ -131,7 +129,7 @@ protected AuthenticationInfo doGetAuthenticationInfo(final AuthenticationToken t
131129
return sai;
132130
} else {
133131
// Bad password
134-
throw new IncorrectCredentialsException();
132+
throw new IncorrectCredentialsException("Incorrect credentials");
135133
}
136134
}
137135

‎plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/User.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@
1212

1313
/**
1414
* A non-volatile and immutable object in the storage.
15-
*
16-
* @opensearch.experimental
1715
*/
1816
public class User {
1917

‎plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroIdentityPluginTests.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,35 +13,35 @@
1313
import org.opensearch.identity.IdentityService;
1414
import org.opensearch.plugins.IdentityPlugin;
1515
import org.opensearch.test.OpenSearchTestCase;
16-
import org.opensearch.threadpool.TestThreadPool;
16+
import org.opensearch.threadpool.ThreadPool;
1717

1818
import java.util.List;
1919

2020
import static org.hamcrest.MatcherAssert.assertThat;
2121
import static org.hamcrest.Matchers.instanceOf;
2222
import static org.hamcrest.Matchers.is;
2323
import static org.junit.Assert.assertThrows;
24+
import static org.mockito.Mockito.mock;
2425

2526
public class ShiroIdentityPluginTests extends OpenSearchTestCase {
2627

2728
public void testSingleIdentityPluginSucceeds() {
28-
TestThreadPool threadPool = new TestThreadPool(getTestName());
2929
IdentityPlugin identityPlugin1 = new ShiroIdentityPlugin(Settings.EMPTY);
3030
List<IdentityPlugin> pluginList1 = List.of(identityPlugin1);
31-
IdentityService identityService1 = new IdentityService(Settings.EMPTY, threadPool, pluginList1);
31+
IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList1);
3232
assertThat(identityService1.getTokenManager(), is(instanceOf(ShiroTokenManager.class)));
33-
terminate(threadPool);
3433
}
3534

3635
public void testMultipleIdentityPluginsFail() {
37-
TestThreadPool threadPool = new TestThreadPool(getTestName());
3836
IdentityPlugin identityPlugin1 = new ShiroIdentityPlugin(Settings.EMPTY);
3937
IdentityPlugin identityPlugin2 = new ShiroIdentityPlugin(Settings.EMPTY);
4038
IdentityPlugin identityPlugin3 = new ShiroIdentityPlugin(Settings.EMPTY);
4139
List<IdentityPlugin> pluginList = List.of(identityPlugin1, identityPlugin2, identityPlugin3);
42-
Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, threadPool, pluginList));
40+
Exception ex = assertThrows(
41+
OpenSearchException.class,
42+
() -> new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList)
43+
);
4344
assert (ex.getMessage().contains("Multiple identity plugins are not supported,"));
44-
terminate(threadPool);
4545
}
4646

4747
}

‎plugins/identity-shiro/src/test/java/org/opensearch/identity/shiro/ShiroTokenExtractorTests.java

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
9+
package org.opensearch.identity.shiro;
10+
11+
import org.opensearch.identity.tokens.AuthToken;
12+
import org.opensearch.identity.tokens.BasicAuthToken;
13+
import org.opensearch.rest.RestRequest;
14+
import org.opensearch.test.OpenSearchTestCase;
15+
import org.opensearch.test.rest.FakeRestRequest;
16+
17+
import java.nio.charset.StandardCharsets;
18+
import java.util.Base64;
19+
import java.util.List;
20+
import java.util.Map;
21+
22+
import static org.hamcrest.Matchers.equalTo;
23+
import static org.hamcrest.Matchers.instanceOf;
24+
25+
public class ShiroTokenExtractorTests extends OpenSearchTestCase {
26+
27+
public void testAuthorizationHeaderExtractionWithBasicAuthToken() {
28+
String basicAuthHeader = Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
29+
RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(
30+
Map.of(ShiroTokenExtractor.AUTH_HEADER_NAME, List.of(BasicAuthToken.TOKEN_IDENTIFIER + " " + basicAuthHeader))
31+
).build();
32+
AuthToken extractedToken = ShiroTokenExtractor.extractToken(fakeRequest);
33+
assertThat(extractedToken, instanceOf(BasicAuthToken.class));
34+
assertThat(extractedToken.asAuthHeaderValue(), equalTo(basicAuthHeader));
35+
}
36+
37+
public void testAuthorizationHeaderExtractionWithUnknownToken() {
38+
String authHeader = "foo";
39+
RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(
40+
Map.of(ShiroTokenExtractor.AUTH_HEADER_NAME, List.of(authHeader))
41+
).build();
42+
AuthToken extractedToken = ShiroTokenExtractor.extractToken(fakeRequest);
43+
assertNull(extractedToken);
44+
}
45+
}

‎server/src/main/java/org/opensearch/action/ActionModule.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -579,7 +579,7 @@ public ActionModule(
579579
actionPlugins.stream().flatMap(p -> p.indicesAliasesRequestValidators().stream()).collect(Collectors.toList())
580580
);
581581

582-
restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService, identityService);
582+
restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService);
583583
}
584584

585585
public Map<String, ActionHandler<?, ?>> getActions() {

‎server/src/main/java/org/opensearch/rest/RestController.java

Lines changed: 2 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import org.opensearch.common.io.stream.BytesStreamOutput;
4242
import org.opensearch.common.logging.DeprecationLogger;
4343
import org.opensearch.common.path.PathTrie;
44-
import org.opensearch.common.util.FeatureFlags;
4544
import org.opensearch.common.util.concurrent.ThreadContext;
4645
import org.opensearch.common.util.io.Streams;
4746
import org.opensearch.common.xcontent.XContentType;
@@ -56,11 +55,6 @@
5655
import org.opensearch.core.xcontent.XContentBuilder;
5756
import org.opensearch.http.HttpChunk;
5857
import org.opensearch.http.HttpServerTransport;
59-
import org.opensearch.identity.IdentityService;
60-
import org.opensearch.identity.Subject;
61-
import org.opensearch.identity.UserSubject;
62-
import org.opensearch.identity.tokens.AuthToken;
63-
import org.opensearch.identity.tokens.RestTokenExtractor;
6458
import org.opensearch.usage.UsageService;
6559

6660
import java.io.ByteArrayOutputStream;
@@ -125,25 +119,23 @@ public class RestController implements HttpServerTransport.Dispatcher {
125119
/** Rest headers that are copied to internal requests made during a rest request. */
126120
private final Set<RestHeaderDefinition> headersToCopy;
127121
private final UsageService usageService;
128-
private final IdentityService identityService;
129122

130123
public RestController(
131124
Set<RestHeaderDefinition> headersToCopy,
132125
UnaryOperator<RestHandler> handlerWrapper,
133126
NodeClient client,
134127
CircuitBreakerService circuitBreakerService,
135-
UsageService usageService,
136-
IdentityService identityService
128+
UsageService usageService
137129
) {
138130
this.headersToCopy = headersToCopy;
139131
this.usageService = usageService;
140132
if (handlerWrapper == null) {
141133
handlerWrapper = h -> h; // passthrough if no wrapper set
142134
}
135+
143136
this.handlerWrapper = handlerWrapper;
144137
this.client = client;
145138
this.circuitBreakerService = circuitBreakerService;
146-
this.identityService = identityService;
147139
registerHandlerNoWrap(
148140
RestRequest.Method.GET,
149141
"/favicon.ico",
@@ -472,11 +464,6 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
472464
return;
473465
}
474466
} else {
475-
if (FeatureFlags.isEnabled(FeatureFlags.IDENTITY)) {
476-
if (!handleAuthenticateUser(request, channel)) {
477-
return;
478-
}
479-
}
480467
dispatchRequest(request, channel, handler);
481468
return;
482469
}
@@ -587,43 +574,6 @@ private void handleBadRequest(String uri, RestRequest.Method method, RestChannel
587574
}
588575
}
589576

590-
/**
591-
* Attempts to extract auth token and login.
592-
*
593-
* @return false if there was an error and the request should not continue being dispatched
594-
* */
595-
private boolean handleAuthenticateUser(final RestRequest request, final RestChannel channel) {
596-
try {
597-
final AuthToken token = RestTokenExtractor.extractToken(request);
598-
// If no token was found, continue executing the request
599-
if (token == null) {
600-
// Authentication did not fail so return true. Authorization is handled at the action level.
601-
return true;
602-
}
603-
final Subject currentSubject = identityService.getCurrentSubject();
604-
if (currentSubject instanceof UserSubject) {
605-
((UserSubject) currentSubject).authenticate(token);
606-
logger.debug("Logged in as user " + currentSubject);
607-
}
608-
} catch (final Exception e) {
609-
try {
610-
final BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(
611-
channel,
612-
RestStatus.UNAUTHORIZED,
613-
e.getMessage()
614-
);
615-
channel.sendResponse(bytesRestResponse);
616-
} catch (final Exception ex) {
617-
final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, ex.getMessage());
618-
channel.sendResponse(bytesRestResponse);
619-
}
620-
return false;
621-
}
622-
623-
// Authentication did not fail so return true. Authorization is handled at the action level.
624-
return true;
625-
}
626-
627577
/**
628578
* Get the valid set of HTTP methods for a REST request.
629579
*/

0 commit comments

Comments
 (0)