Skip to content

Commit 89910d9

Browse files
opensearch-trigger-bot[bot]github-actions[bot]
opensearch-trigger-bot[bot]
and
github-actions[bot]
authored
[Backport 2.x] Use DelegatingRestHandler to delegate to the original handler (#3547)
Backport 1166c1f from #3517. Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent b5622c4 commit 89910d9

4 files changed

Lines changed: 199 additions & 51 deletions

File tree

‎src/main/java/org/opensearch/security/filter/DelegatingRestHandler.java‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
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.security.filter;
10+
11+
import org.opensearch.client.node.NodeClient;
12+
import org.opensearch.rest.RestChannel;
13+
import org.opensearch.rest.RestHandler;
14+
import org.opensearch.rest.RestRequest;
15+
16+
import java.util.List;
17+
import java.util.Objects;
18+
19+
/**
20+
* Delegating RestHandler that delegates all implementations to original handler
21+
*
22+
* @opensearch.api
23+
*/
24+
public class DelegatingRestHandler implements RestHandler {
25+
26+
protected final RestHandler delegate;
27+
28+
public DelegatingRestHandler(RestHandler delegate) {
29+
Objects.requireNonNull(delegate, "RestHandler delegate can not be null");
30+
this.delegate = delegate;
31+
}
32+
33+
@Override
34+
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
35+
delegate.handleRequest(request, channel, client);
36+
}
37+
38+
@Override
39+
public boolean canTripCircuitBreaker() {
40+
return delegate.canTripCircuitBreaker();
41+
}
42+
43+
@Override
44+
public boolean supportsContentStream() {
45+
return delegate.supportsContentStream();
46+
}
47+
48+
@Override
49+
public boolean allowsUnsafeBuffers() {
50+
return delegate.allowsUnsafeBuffers();
51+
}
52+
53+
@Override
54+
public List<Route> routes() {
55+
return delegate.routes();
56+
}
57+
58+
@Override
59+
public List<DeprecatedRoute> deprecatedRoutes() {
60+
return delegate.deprecatedRoutes();
61+
}
62+
63+
@Override
64+
public List<ReplacedRoute> replacedRoutes() {
65+
return delegate.replacedRoutes();
66+
}
67+
68+
@Override
69+
public boolean allowSystemIndexAccessByDefault() {
70+
return delegate.allowSystemIndexAccessByDefault();
71+
}
72+
73+
@Override
74+
public String toString() {
75+
return delegate.toString();
76+
}
77+
}

‎src/main/java/org/opensearch/security/filter/SecurityRestFilter.java‎

Lines changed: 33 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,13 @@
4141
import org.greenrobot.eventbus.Subscribe;
4242

4343
import org.opensearch.OpenSearchException;
44+
import org.opensearch.client.node.NodeClient;
4445
import org.opensearch.common.settings.Settings;
4546
import org.opensearch.common.util.concurrent.ThreadContext;
4647
import org.opensearch.rest.NamedRoute;
48+
import org.opensearch.rest.RestChannel;
4749
import org.opensearch.rest.RestHandler;
50+
import org.opensearch.rest.RestRequest;
4851
import org.opensearch.rest.RestRequest.Method;
4952
import org.opensearch.security.auditlog.AuditLog;
5053
import org.opensearch.security.auditlog.AuditLog.Origin;
@@ -116,22 +119,16 @@ public SecurityRestFilter(
116119
this.allowlistingSettings = new AllowlistingSettings();
117120
}
118121

119-
/**
120-
* This function wraps around all rest requests
121-
* If the request is authenticated, then it goes through a allowlisting check.
122-
* The allowlisting check works as follows:
123-
* If allowlisting is not enabled, then requests are handled normally.
124-
* If allowlisting is enabled, then SuperAdmin is allowed access to all APIs, regardless of what is currently allowlisted.
125-
* If allowlisting is enabled, then Non-SuperAdmin is allowed to access only those APIs that are allowlisted in {@link #requests}
126-
* For example: if allowlisting is enabled and requests = ["/_cat/nodes"], then SuperAdmin can access all APIs, but non SuperAdmin
127-
* can only access "/_cat/nodes"
128-
* Further note: Some APIs are only accessible by SuperAdmin, regardless of allowlisting. For example: /_opendistro/_security/api/whitelist is only accessible by SuperAdmin.
129-
* See {@link AllowlistApiAction} for the implementation of this API.
130-
* SuperAdmin is identified by credentials, which can be passed in the curl request.
131-
*/
132-
public RestHandler wrap(RestHandler original, AdminDNs adminDNs) {
133-
return (request, channel, client) -> {
122+
class AuthczRestHandler extends DelegatingRestHandler {
123+
private final AdminDNs adminDNs;
124+
125+
public AuthczRestHandler(RestHandler original, AdminDNs adminDNs) {
126+
super(original);
127+
this.adminDNs = adminDNs;
128+
}
134129

130+
@Override
131+
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
135132
final Optional<SecurityResponse> maybeSavedResponse = NettyAttribute.popFrom(request, EARLY_RESPONSE);
136133
if (maybeSavedResponse.isPresent()) {
137134
NettyAttribute.clearAttribute(request, CONTEXT_TO_RESTORE);
@@ -165,7 +162,7 @@ public RestHandler wrap(RestHandler original, AdminDNs adminDNs) {
165162
final User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
166163
if (userIsSuperAdmin(user, adminDNs)) {
167164
// Super admins are always authorized
168-
original.handleRequest(request, channel, client);
165+
delegate.handleRequest(request, channel, client);
169166
return;
170167
}
171168

@@ -177,15 +174,32 @@ public RestHandler wrap(RestHandler original, AdminDNs adminDNs) {
177174
return;
178175
}
179176

180-
authorizeRequest(original, requestChannel, user);
177+
authorizeRequest(delegate, requestChannel, user);
181178
if (requestChannel.getQueuedResponse().isPresent()) {
182179
channel.sendResponse(requestChannel.getQueuedResponse().get().asRestResponse());
183180
return;
184181
}
185182

186183
// Caller was authorized, forward the request to the handler
187-
original.handleRequest(request, channel, client);
188-
};
184+
delegate.handleRequest(request, channel, client);
185+
}
186+
}
187+
188+
/**
189+
* This function wraps around all rest requests
190+
* If the request is authenticated, then it goes through a allowlisting check.
191+
* The allowlisting check works as follows:
192+
* If allowlisting is not enabled, then requests are handled normally.
193+
* If allowlisting is enabled, then SuperAdmin is allowed access to all APIs, regardless of what is currently allowlisted.
194+
* If allowlisting is enabled, then Non-SuperAdmin is allowed to access only those APIs that are allowlisted in {@link #requests}
195+
* For example: if allowlisting is enabled and requests = ["/_cat/nodes"], then SuperAdmin can access all APIs, but non SuperAdmin
196+
* can only access "/_cat/nodes"
197+
* Further note: Some APIs are only accessible by SuperAdmin, regardless of allowlisting. For example: /_opendistro/_security/api/whitelist is only accessible by SuperAdmin.
198+
* See {@link AllowlistApiAction} for the implementation of this API.
199+
* SuperAdmin is identified by credentials, which can be passed in the curl request.
200+
*/
201+
public RestHandler wrap(RestHandler original, AdminDNs adminDNs) {
202+
return new AuthczRestHandler(original, adminDNs);
189203
}
190204

191205
/**

‎src/test/java/org/opensearch/security/filter/DelegatingRestHandlerTests.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
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.security.filter;
10+
11+
import org.junit.Test;
12+
import org.opensearch.client.node.NodeClient;
13+
import org.opensearch.core.common.bytes.BytesArray;
14+
import org.opensearch.core.rest.RestStatus;
15+
import org.opensearch.rest.BytesRestResponse;
16+
import org.opensearch.rest.RestChannel;
17+
import org.opensearch.rest.RestHandler;
18+
import org.opensearch.rest.RestRequest;
19+
20+
import java.lang.reflect.Method;
21+
import java.lang.reflect.Modifier;
22+
import java.util.Arrays;
23+
import java.util.List;
24+
import java.util.stream.Collectors;
25+
26+
import static org.mockito.ArgumentMatchers.any;
27+
import static org.mockito.Mockito.spy;
28+
import static org.mockito.Mockito.times;
29+
import static org.mockito.Mockito.verify;
30+
import static org.mockito.Mockito.verifyNoMoreInteractions;
31+
32+
public class DelegatingRestHandlerTests {
33+
34+
@Test
35+
public void testDelegatingRestHandlerShouldActAsOriginal() throws Exception {
36+
RestHandler rh = new RestHandler() {
37+
@Override
38+
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
39+
new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY);
40+
}
41+
};
42+
RestHandler handlerSpy = spy(rh);
43+
DelegatingRestHandler drh = new DelegatingRestHandler(handlerSpy);
44+
45+
List<Method> overridableMethods = Arrays.stream(RestHandler.class.getMethods())
46+
.filter(
47+
m -> !(Modifier.isPrivate(m.getModifiers()) || Modifier.isStatic(m.getModifiers()) || Modifier.isFinal(m.getModifiers()))
48+
)
49+
.collect(Collectors.toList());
50+
51+
for (Method method : overridableMethods) {
52+
int argCount = method.getParameterCount();
53+
Object[] args = new Object[argCount];
54+
for (int i = 0; i < argCount; i++) {
55+
args[i] = any();
56+
}
57+
if (args.length > 0) {
58+
method.invoke(drh, args);
59+
} else {
60+
method.invoke(drh);
61+
}
62+
method.invoke(verify(handlerSpy, times(1)), args);
63+
}
64+
verifyNoMoreInteractions(handlerSpy);
65+
}
66+
}

‎src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java‎

Lines changed: 23 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,23 @@
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+
* Modifications Copyright OpenSearch Contributors. See
9+
* GitHub history for details.
10+
*/
11+
112
package org.opensearch.security.filter;
213

314
import org.junit.Before;
4-
import org.junit.Ignore;
515
import org.junit.Test;
616
import org.opensearch.client.node.NodeClient;
717
import org.opensearch.common.settings.Settings;
818
import org.opensearch.common.util.concurrent.ThreadContext;
919
import org.opensearch.core.common.bytes.BytesArray;
1020
import org.opensearch.core.rest.RestStatus;
11-
import org.opensearch.core.xcontent.NamedXContentRegistry;
1221
import org.opensearch.rest.BytesRestResponse;
1322
import org.opensearch.rest.RestChannel;
1423
import org.opensearch.rest.RestHandler;
@@ -19,17 +28,15 @@
1928
import org.opensearch.security.configuration.CompatConfig;
2029
import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator;
2130
import org.opensearch.security.ssl.transport.PrincipalExtractor;
22-
import org.opensearch.test.rest.FakeRestRequest;
2331
import org.opensearch.threadpool.ThreadPool;
2432

2533
import java.nio.file.Path;
26-
import java.util.List;
27-
import java.util.Map;
2834

35+
import static org.junit.Assert.assertFalse;
36+
import static org.junit.Assert.assertTrue;
2937
import static org.mockito.ArgumentMatchers.any;
3038
import static org.mockito.Mockito.doReturn;
3139
import static org.mockito.Mockito.mock;
32-
import static org.mockito.Mockito.never;
3340
import static org.mockito.Mockito.spy;
3441
import static org.mockito.Mockito.verify;
3542

@@ -65,47 +72,31 @@ public void setUp() throws NoSuchMethodException {
6572
);
6673
}
6774

68-
@Ignore
75+
/**
76+
* Tests to ensure that the output of {@link SecurityRestFilter#wrap} is an instance of AuthczRestHandler
77+
*/
6978
@Test
70-
public void testDoesCallDelegateOnSuccessfulAuthorization() throws Exception {
71-
SecurityRestFilter filterSpy = spy(sf);
79+
public void testSecurityRestFilterWrap() throws Exception {
7280
AdminDNs adminDNs = mock(AdminDNs.class);
7381

74-
RestHandler testRestHandlerSpy = spy(testRestHandler);
75-
RestHandler wrappedRestHandler = filterSpy.wrap(testRestHandlerSpy, adminDNs);
76-
77-
doReturn(false).when(filterSpy).userIsSuperAdmin(any(), any());
78-
// doReturn(true).when(filterSpy).authorizeRequest(any(), any(), any());
82+
RestHandler wrappedRestHandler = sf.wrap(testRestHandler, adminDNs);
7983

80-
FakeRestRequest fakeRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withPath("/test")
81-
.withMethod(RestRequest.Method.POST)
82-
.withHeaders(Map.of("Content-Type", List.of("application/json")))
83-
.build();
84-
85-
wrappedRestHandler.handleRequest(fakeRequest, mock(RestChannel.class), mock(NodeClient.class));
86-
87-
verify(testRestHandlerSpy).handleRequest(any(), any(), any());
84+
assertTrue(wrappedRestHandler instanceof SecurityRestFilter.AuthczRestHandler);
85+
assertFalse(wrappedRestHandler instanceof TestRestHandler);
8886
}
8987

90-
@Ignore
9188
@Test
92-
public void testDoesNotCallDelegateOnUnauthorized() throws Exception {
89+
public void testDoesCallDelegateOnSuccessfulAuthorization() throws Exception {
9390
SecurityRestFilter filterSpy = spy(sf);
9491
AdminDNs adminDNs = mock(AdminDNs.class);
9592

9693
RestHandler testRestHandlerSpy = spy(testRestHandler);
9794
RestHandler wrappedRestHandler = filterSpy.wrap(testRestHandlerSpy, adminDNs);
9895

9996
doReturn(false).when(filterSpy).userIsSuperAdmin(any(), any());
100-
// doReturn(false).when(filterSpy).authorizeRequest(any(), any(), any());
101-
102-
FakeRestRequest fakeRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withPath("/test")
103-
.withMethod(RestRequest.Method.POST)
104-
.withHeaders(Map.of("Content-Type", List.of("application/json")))
105-
.build();
10697

107-
wrappedRestHandler.handleRequest(fakeRequest, mock(RestChannel.class), mock(NodeClient.class));
98+
wrappedRestHandler.handleRequest(mock(RestRequest.class), mock(RestChannel.class), mock(NodeClient.class));
10899

109-
verify(testRestHandlerSpy, never()).handleRequest(any(), any(), any());
100+
verify(testRestHandlerSpy).handleRequest(any(), any(), any());
110101
}
111102
}

0 commit comments

Comments
 (0)