Skip to content

Commit e85a38b

Browse files
mengweiericmengweieric
committed
address issue: transport action wrapper
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent 94f29ef commit e85a38b

5 files changed

Lines changed: 159 additions & 9 deletions

File tree

‎integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,23 @@ private JSONObject executeQueryAsUser(String query, String username) throws IOEx
339339
return new JSONObject(org.opensearch.sql.legacy.TestUtils.getResponseBody(response, true));
340340
}
341341

342+
/** Executes a grammar metadata request as a specific user with basic authentication. */
343+
private JSONObject executeGrammarAsUser(String username) throws IOException {
344+
Request request = new Request("GET", "/_plugins/_ppl/_grammar");
345+
346+
RequestOptions.Builder restOptionsBuilder = RequestOptions.DEFAULT.toBuilder();
347+
restOptionsBuilder.addHeader(
348+
"Authorization",
349+
"Basic "
350+
+ java.util.Base64.getEncoder()
351+
.encodeToString((username + ":" + STRONG_PASSWORD).getBytes()));
352+
request.setOptions(restOptionsBuilder);
353+
354+
Response response = client().performRequest(request);
355+
assertEquals(200, response.getStatusLine().getStatusCode());
356+
return new JSONObject(org.opensearch.sql.legacy.TestUtils.getResponseBody(response, true));
357+
}
358+
342359
@Test
343360
public void testUserWithBankPermissionCanAccessBankIndex() throws IOException {
344361
// Test that bank_user can access bank index - this should work with the fix
@@ -512,6 +529,32 @@ public void testBankUserWithEvalCommand() throws IOException {
512529
verifyColumn(result, columnName("full_name"));
513530
}
514531

532+
@Test
533+
public void testUserWithPPLPermissionCanAccessGrammarEndpoint() throws IOException {
534+
JSONObject result = executeGrammarAsUser(BANK_USER);
535+
assertTrue(result.has("bundleVersion"));
536+
assertTrue(result.has("antlrVersion"));
537+
assertTrue(result.has("grammarHash"));
538+
assertTrue(result.has("tokenDictionary"));
539+
}
540+
541+
@Test
542+
public void testUserWithoutPPLPermissionCannotAccessGrammarEndpoint() throws IOException {
543+
try {
544+
executeGrammarAsUser(NO_PPL_USER);
545+
fail("Expected security exception for user without PPL permission");
546+
} catch (ResponseException e) {
547+
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
548+
String responseBody =
549+
org.opensearch.sql.legacy.TestUtils.getResponseBody(e.getResponse(), false);
550+
assertTrue(
551+
"Response should contain permission error message",
552+
responseBody.contains("no permissions")
553+
|| responseBody.contains("Forbidden")
554+
|| responseBody.contains("cluster:admin/opensearch/ppl"));
555+
}
556+
}
557+
515558
// Negative test cases for missing permissions
516559

517560
@Test

‎plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLGrammarAction.java‎

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,16 @@
1313
import java.util.List;
1414
import lombok.extern.log4j.Log4j2;
1515
import org.opensearch.common.annotation.ExperimentalApi;
16+
import org.opensearch.core.action.ActionListener;
1617
import org.opensearch.core.rest.RestStatus;
1718
import org.opensearch.core.xcontent.XContentBuilder;
1819
import org.opensearch.rest.BaseRestHandler;
1920
import org.opensearch.rest.BytesRestResponse;
21+
import org.opensearch.rest.RestChannel;
2022
import org.opensearch.rest.RestRequest;
23+
import org.opensearch.sql.plugin.transport.PPLQueryAction;
24+
import org.opensearch.sql.plugin.transport.TransportPPLQueryRequest;
25+
import org.opensearch.sql.plugin.transport.TransportPPLQueryResponse;
2126
import org.opensearch.sql.ppl.autocomplete.GrammarBundle;
2227
import org.opensearch.sql.ppl.autocomplete.PPLGrammarBundleBuilder;
2328
import org.opensearch.transport.client.node.NodeClient;
@@ -52,17 +57,50 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
5257

5358
return channel -> {
5459
try {
55-
GrammarBundle bundle = getOrBuildBundle();
56-
XContentBuilder builder = channel.newBuilder();
57-
serializeBundle(builder, bundle);
58-
channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder));
60+
authorizeRequest(
61+
client,
62+
new ActionListener<>() {
63+
@Override
64+
public void onResponse(TransportPPLQueryResponse ignored) {
65+
try {
66+
GrammarBundle bundle = getOrBuildBundle();
67+
XContentBuilder builder = channel.newBuilder();
68+
serializeBundle(builder, bundle);
69+
channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder));
70+
} catch (Exception e) {
71+
log.error("Error building or serializing PPL grammar", e);
72+
sendErrorResponse(channel, e);
73+
}
74+
}
75+
76+
@Override
77+
public void onFailure(Exception e) {
78+
log.error("PPL grammar authorization failed", e);
79+
sendErrorResponse(channel, e);
80+
}
81+
});
5982
} catch (Exception e) {
60-
log.error("Error building or serializing PPL grammar", e);
61-
channel.sendResponse(new BytesRestResponse(channel, e));
83+
log.error("Error authorizing PPL grammar request", e);
84+
sendErrorResponse(channel, e);
6285
}
6386
};
6487
}
6588

89+
@VisibleForTesting
90+
protected void authorizeRequest(
91+
NodeClient client, ActionListener<TransportPPLQueryResponse> listener) {
92+
client.execute(
93+
PPLQueryAction.INSTANCE, new TransportPPLQueryRequest("", null, ENDPOINT_PATH), listener);
94+
}
95+
96+
private void sendErrorResponse(RestChannel channel, Exception e) {
97+
try {
98+
channel.sendResponse(new BytesRestResponse(channel, e));
99+
} catch (IOException ioException) {
100+
log.error("Failed to send PPL grammar error response", ioException);
101+
}
102+
}
103+
66104
// Thread-safe lazy initialization.
67105
private synchronized GrammarBundle getOrBuildBundle() {
68106
if (cachedBundle == null) {

‎plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,13 +100,21 @@ protected void doExecute(
100100
+ " false"));
101101
return;
102102
}
103+
104+
TransportPPLQueryRequest transportRequest = TransportPPLQueryRequest.fromActionRequest(request);
105+
if (transportRequest.isGrammarRequest()) {
106+
// Authorization is enforced by this transport action before returning grammar metadata in
107+
// REST.
108+
listener.onResponse(new TransportPPLQueryResponse("{}"));
109+
return;
110+
}
111+
103112
Metrics.getInstance().getNumericalMetric(MetricName.PPL_REQ_TOTAL).increment();
104113
Metrics.getInstance().getNumericalMetric(MetricName.PPL_REQ_COUNT_TOTAL).increment();
105114

106115
QueryContext.addRequestId();
107116

108117
PPLService pplService = injector.getInstance(PPLService.class);
109-
TransportPPLQueryRequest transportRequest = TransportPPLQueryRequest.fromActionRequest(request);
110118
// in order to use PPL service, we need to convert TransportPPLQueryRequest to PPLQueryRequest
111119
PPLQueryRequest transformedRequest = transportRequest.toPPLQueryRequest();
112120
QueryContext.setProfile(transformedRequest.profile());

‎plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,16 @@ public String getRequest() {
119119
* @return true if it is an explain request
120120
*/
121121
public boolean isExplainRequest() {
122-
return path.endsWith("/_explain");
122+
return path != null && path.endsWith("/_explain");
123+
}
124+
125+
/**
126+
* Check if request is for grammar metadata endpoint.
127+
*
128+
* @return true if it is a grammar metadata request
129+
*/
130+
public boolean isGrammarRequest() {
131+
return path != null && path.endsWith("/_grammar");
123132
}
124133

125134
/** Decide on the formatter by the requested format. */

‎plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java‎

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,18 @@
1515
import org.json.JSONObject;
1616
import org.junit.Before;
1717
import org.junit.Test;
18+
import org.opensearch.OpenSearchStatusException;
1819
import org.opensearch.common.io.stream.BytesStreamOutput;
1920
import org.opensearch.common.xcontent.XContentType;
21+
import org.opensearch.core.action.ActionListener;
2022
import org.opensearch.core.rest.RestStatus;
2123
import org.opensearch.core.xcontent.MediaType;
2224
import org.opensearch.core.xcontent.NamedXContentRegistry;
2325
import org.opensearch.core.xcontent.XContentBuilder;
2426
import org.opensearch.rest.RestChannel;
2527
import org.opensearch.rest.RestRequest;
2628
import org.opensearch.rest.RestResponse;
29+
import org.opensearch.sql.plugin.transport.TransportPPLQueryResponse;
2730
import org.opensearch.sql.ppl.autocomplete.GrammarBundle;
2831
import org.opensearch.test.rest.FakeRestRequest;
2932
import org.opensearch.transport.client.node.NodeClient;
@@ -36,7 +39,14 @@ public class RestPPLGrammarActionTest {
3639

3740
@Before
3841
public void setUp() {
39-
action = new RestPPLGrammarAction();
42+
action =
43+
new RestPPLGrammarAction() {
44+
@Override
45+
protected void authorizeRequest(
46+
NodeClient client, ActionListener<TransportPPLQueryResponse> listener) {
47+
listener.onResponse(new TransportPPLQueryResponse("{}"));
48+
}
49+
};
4050
client = mock(NodeClient.class);
4151
}
4252

@@ -101,6 +111,12 @@ public void testGetGrammar_BundleIsCached() throws Exception {
101111
AtomicInteger buildCount = new AtomicInteger(0);
102112
RestPPLGrammarAction countingAction =
103113
new RestPPLGrammarAction() {
114+
@Override
115+
protected void authorizeRequest(
116+
NodeClient client, ActionListener<TransportPPLQueryResponse> listener) {
117+
listener.onResponse(new TransportPPLQueryResponse("{}"));
118+
}
119+
104120
@Override
105121
protected GrammarBundle buildBundle() {
106122
buildCount.incrementAndGet();
@@ -128,6 +144,12 @@ public void testInvalidateCache() throws Exception {
128144
AtomicInteger buildCount = new AtomicInteger(0);
129145
RestPPLGrammarAction countingAction =
130146
new RestPPLGrammarAction() {
147+
@Override
148+
protected void authorizeRequest(
149+
NodeClient client, ActionListener<TransportPPLQueryResponse> listener) {
150+
listener.onResponse(new TransportPPLQueryResponse("{}"));
151+
}
152+
131153
@Override
132154
protected GrammarBundle buildBundle() {
133155
buildCount.incrementAndGet();
@@ -154,6 +176,12 @@ protected GrammarBundle buildBundle() {
154176
public void testGetGrammar_ErrorPath_Returns500() throws Exception {
155177
RestPPLGrammarAction failingAction =
156178
new RestPPLGrammarAction() {
179+
@Override
180+
protected void authorizeRequest(
181+
NodeClient client, ActionListener<TransportPPLQueryResponse> listener) {
182+
listener.onResponse(new TransportPPLQueryResponse("{}"));
183+
}
184+
157185
@Override
158186
protected GrammarBundle buildBundle() {
159187
throw new RuntimeException("simulated build failure");
@@ -171,6 +199,12 @@ protected GrammarBundle buildBundle() {
171199
public void testGetGrammar_NullBundle_Returns500() throws Exception {
172200
RestPPLGrammarAction nullBundleAction =
173201
new RestPPLGrammarAction() {
202+
@Override
203+
protected void authorizeRequest(
204+
NodeClient client, ActionListener<TransportPPLQueryResponse> listener) {
205+
listener.onResponse(new TransportPPLQueryResponse("{}"));
206+
}
207+
174208
@Override
175209
protected GrammarBundle buildBundle() {
176210
return null;
@@ -184,6 +218,24 @@ protected GrammarBundle buildBundle() {
184218
assertEquals(RestStatus.INTERNAL_SERVER_ERROR, channel.getResponse().status());
185219
}
186220

221+
@Test
222+
public void testGetGrammar_AuthorizationFailure_Returns403() throws Exception {
223+
RestPPLGrammarAction unauthorizedAction =
224+
new RestPPLGrammarAction() {
225+
@Override
226+
protected void authorizeRequest(
227+
NodeClient client, ActionListener<TransportPPLQueryResponse> listener) {
228+
listener.onFailure(new OpenSearchStatusException("forbidden", RestStatus.FORBIDDEN));
229+
}
230+
};
231+
232+
FakeRestRequest request = newGrammarGetRequest();
233+
MockRestChannel channel = new MockRestChannel(request, true);
234+
unauthorizedAction.handleRequest(request, channel, client);
235+
236+
assertEquals(RestStatus.FORBIDDEN, channel.getResponse().status());
237+
}
238+
187239
private static FakeRestRequest newGrammarGetRequest() {
188240
return new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
189241
.withMethod(RestRequest.Method.GET)

0 commit comments

Comments
 (0)