Skip to content

Commit c0ac784

Browse files
mengweiericmengweieric
authored
[BugFix] Scope SQL cursor continuation to original query indices under FGAC (opensearch-project#5399)
1 parent d3bdca8 commit c0ac784

5 files changed

Lines changed: 311 additions & 1 deletion

File tree

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

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.security;
7+
8+
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT;
9+
10+
import java.io.IOException;
11+
import java.util.Base64;
12+
import java.util.Locale;
13+
import org.json.JSONObject;
14+
import org.junit.Test;
15+
import org.opensearch.client.Request;
16+
import org.opensearch.client.RequestOptions;
17+
import org.opensearch.client.Response;
18+
import org.opensearch.sql.legacy.SQLIntegTestCase;
19+
import org.opensearch.sql.legacy.TestUtils;
20+
21+
/**
22+
* Regression test for SQL cursor pagination under Fine-Grained Access Control (FGAC).
23+
*
24+
* <p>Exercises the legacy V1 cursor path (triggered by {@code SELECT ... LIMIT n} with {@code
25+
* fetch_size}). Before the fix, page 2 would return 403 because the continuation SearchRequest was
26+
* created with no indices, which Security resolves to a wildcard and denies under FGAC.
27+
*/
28+
public class SQLCursorPermissionsIT extends SQLIntegTestCase {
29+
30+
private static final String ACCOUNT_USER = "account_cursor_user";
31+
private static final String ACCOUNT_ROLE = "account_cursor_role";
32+
private static final String STRONG_PASSWORD = "StrongPassword123!";
33+
34+
private boolean initialized = false;
35+
36+
@Override
37+
protected void init() throws Exception {
38+
loadIndex(Index.ACCOUNT);
39+
createSecurityRolesAndUsers();
40+
}
41+
42+
private void createSecurityRolesAndUsers() throws IOException {
43+
if (initialized) {
44+
return;
45+
}
46+
createRole(ACCOUNT_ROLE, TEST_INDEX_ACCOUNT);
47+
createUser(ACCOUNT_USER, ACCOUNT_ROLE);
48+
initialized = true;
49+
}
50+
51+
private void createRole(String roleName, String indexPattern) throws IOException {
52+
Request request = new Request("PUT", "/_plugins/_security/api/roles/" + roleName);
53+
request.setJsonEntity(
54+
String.format(
55+
Locale.ROOT,
56+
"""
57+
{
58+
"cluster_permissions": [
59+
"cluster:admin/opensearch/ppl",
60+
"cluster:admin/opensearch/sql"
61+
],
62+
"index_permissions": [{
63+
"index_patterns": [
64+
"%s"
65+
],
66+
"allowed_actions": [
67+
"indices:data/read/search*",
68+
"indices:admin/mappings/get",
69+
"indices:admin/mappings/fields/get*",
70+
"indices:monitor/settings/get",
71+
"indices:data/read/point_in_time/create",
72+
"indices:data/read/point_in_time/delete"
73+
]
74+
}]
75+
}
76+
""",
77+
indexPattern));
78+
RequestOptions.Builder opts = RequestOptions.DEFAULT.toBuilder();
79+
opts.addHeader("Content-Type", "application/json");
80+
request.setOptions(opts);
81+
82+
Response response = client().performRequest(request);
83+
int status = response.getStatusLine().getStatusCode();
84+
assertTrue(status == 200 || status == 201);
85+
}
86+
87+
private void createUser(String username, String roleName) throws IOException {
88+
Request userRequest = new Request("PUT", "/_plugins/_security/api/internalusers/" + username);
89+
userRequest.setJsonEntity(
90+
String.format(
91+
Locale.ROOT,
92+
"""
93+
{
94+
"password": "%s",
95+
"backend_roles": [],
96+
"attributes": {}
97+
}
98+
""",
99+
STRONG_PASSWORD));
100+
RequestOptions.Builder opts = RequestOptions.DEFAULT.toBuilder();
101+
opts.addHeader("Content-Type", "application/json");
102+
userRequest.setOptions(opts);
103+
104+
Response userResponse = client().performRequest(userRequest);
105+
int userStatus = userResponse.getStatusLine().getStatusCode();
106+
assertTrue(userStatus == 200 || userStatus == 201);
107+
108+
Request mappingRequest = new Request("PUT", "/_plugins/_security/api/rolesmapping/" + roleName);
109+
mappingRequest.setJsonEntity(
110+
String.format(
111+
Locale.ROOT,
112+
"""
113+
{
114+
"backend_roles": [],
115+
"hosts": [],
116+
"users": ["%s"]
117+
}
118+
""",
119+
username));
120+
mappingRequest.setOptions(opts);
121+
122+
Response mappingResponse = client().performRequest(mappingRequest);
123+
int mappingStatus = mappingResponse.getStatusLine().getStatusCode();
124+
assertTrue(mappingStatus == 200 || mappingStatus == 201);
125+
}
126+
127+
private JSONObject executeSqlAsUser(String body, String username) throws IOException {
128+
Request request = new Request("POST", "/_plugins/_sql");
129+
request.setJsonEntity(body);
130+
RequestOptions.Builder opts = RequestOptions.DEFAULT.toBuilder();
131+
opts.addHeader("Content-Type", "application/json");
132+
opts.addHeader(
133+
"Authorization",
134+
"Basic "
135+
+ Base64.getEncoder().encodeToString((username + ":" + STRONG_PASSWORD).getBytes()));
136+
request.setOptions(opts);
137+
138+
Response response = client().performRequest(request);
139+
assertEquals(200, response.getStatusLine().getStatusCode());
140+
return new JSONObject(TestUtils.getResponseBody(response, true));
141+
}
142+
143+
@Test
144+
public void simpleSelectUnderFgacSucceeds() throws IOException {
145+
JSONObject result =
146+
executeSqlAsUser(
147+
String.format(
148+
Locale.ROOT,
149+
"{\"query\": \"SELECT firstname FROM %s LIMIT 1\"}",
150+
TEST_INDEX_ACCOUNT),
151+
ACCOUNT_USER);
152+
assertTrue(result.has("datarows"));
153+
}
154+
155+
/**
156+
* Regression for SQL cursor pagination under FGAC. Triggers the V1 cursor path (LIMIT with
157+
* fetch_size) and advances through multiple continuation pages. Before the fix, page 2 returned
158+
* 403 because the continuation SearchRequest carried no indices, which Security resolves to a
159+
* wildcard and denies.
160+
*/
161+
@Test
162+
public void cursorPaginationUnderFgacSucceedsAcrossPages() throws IOException {
163+
// LIMIT forces the V1 cursor path (V2's CanPaginateVisitor rejects LIMIT). The V1 path is
164+
// the one that constructs the continuation SearchRequest without indices, which Security
165+
// denies under FGAC before this fix.
166+
JSONObject firstPage =
167+
executeSqlAsUser(
168+
String.format(
169+
Locale.ROOT,
170+
"{\"fetch_size\": 50, \"query\": \"SELECT age, balance FROM %s LIMIT 234\"}",
171+
TEST_INDEX_ACCOUNT),
172+
ACCOUNT_USER);
173+
assertTrue("first page must include a cursor; body=" + firstPage, firstPage.has("cursor"));
174+
String cursor = firstPage.getString("cursor");
175+
assertFalse("first page cursor must not be empty", cursor.isEmpty());
176+
assertTrue(
177+
"expected V1 cursor (prefix 'd:'), got: "
178+
+ cursor.substring(0, Math.min(6, cursor.length())),
179+
cursor.startsWith("d:"));
180+
181+
int pages = 1;
182+
while (!cursor.isEmpty()) {
183+
JSONObject next =
184+
executeSqlAsUser(
185+
String.format(Locale.ROOT, "{\"cursor\": \"%s\"}", cursor), ACCOUNT_USER);
186+
cursor = next.optString("cursor", "");
187+
pages++;
188+
}
189+
// 234 rows / 50 per page = 5 pages
190+
assertEquals("expected 5 V1 cursor pages under FGAC", 5, pages);
191+
}
192+
}

‎legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ public class DefaultCursor implements Cursor {
6060
private static final String PIT_ID = "p";
6161
private static final String SEARCH_REQUEST = "r";
6262
private static final String SORT_FIELDS = "h";
63+
private static final String INDICES = "x";
6364
private static final ObjectMapper objectMapper = new ObjectMapper();
6465

6566
/**
@@ -69,6 +70,13 @@ public class DefaultCursor implements Cursor {
6970
*/
7071
@NonNull private String indexPattern;
7172

73+
/**
74+
* Concrete index names from the original query's FROM clause. Used to scope continuation
75+
* SearchRequests so Security FGAC authorizes against the same indices as page 1 instead of a
76+
* wildcard.
77+
*/
78+
private String[] indices;
79+
7280
/**
7381
* List of Schema.Column for maintaining field order and generating null values of missing fields
7482
*/
@@ -137,6 +145,7 @@ public String generateCursorId() {
137145
throw new RuntimeException("Failed to serialize sort fields to JSON string.", e);
138146
}
139147
json.put(SORT_FIELDS, sortFieldValue);
148+
json.put(INDICES, new JSONArray(indices == null ? new String[0] : indices));
140149
setSearchRequestString(json, searchSourceBuilder);
141150

142151
return String.format("%s:%s", type.getId(), encodeCursor(json));
@@ -173,10 +182,23 @@ public static DefaultCursor from(String cursorId) {
173182
populateCursorForPit(json, cursor);
174183
cursor.setColumns(getColumnsFromSchema(json.getJSONArray(SCHEMA_COLUMNS)));
175184
cursor.setFieldAliasMap(fieldAliasMap(json.getJSONObject(FIELD_ALIAS_MAP)));
185+
cursor.setIndices(getIndicesFromJson(json));
176186

177187
return cursor;
178188
}
179189

190+
private static String[] getIndicesFromJson(JSONObject json) {
191+
JSONArray arr = json.optJSONArray(INDICES);
192+
if (arr == null) {
193+
return new String[0];
194+
}
195+
String[] result = new String[arr.length()];
196+
for (int i = 0; i < arr.length(); i++) {
197+
result[i] = arr.getString(i);
198+
}
199+
return result;
200+
}
201+
180202
private static void populateCursorForPit(JSONObject json, DefaultCursor cursor) {
181203
cursor.setPitId(json.getString(PIT_ID));
182204

‎legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,10 @@ private String handleDefaultCursorRequest(Client client, DefaultCursor cursor) {
115115
SearchSourceBuilder source = cursor.getSearchSourceBuilder();
116116
source.searchAfter(cursor.getSortFields());
117117
source.pointInTimeBuilder(new PointInTimeBuilder(pitId));
118-
SearchRequest searchRequest = new SearchRequest();
118+
// Scope continuation to the original query's indices; an empty-indices SearchRequest
119+
// resolves to a wildcard under Security FGAC and gets denied on page 2.
120+
String[] indices = cursor.getIndices();
121+
SearchRequest searchRequest = new SearchRequest(indices == null ? new String[0] : indices);
119122
searchRequest.source(source);
120123
scrollResponse = client.search(searchRequest).actionGet();
121124

‎legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ private DefaultCursor createCursorWithPit(
152152
cursor.setLimit(queryAction.getSelect().getRowCount());
153153
cursor.setFetchSize(fetchSize);
154154
cursor.setPitId(pit.getPitId());
155+
cursor.setIndices(queryAction.getSelect().getIndexArr());
155156
cursor.setSearchSourceBuilder(queryAction.getRequestBuilder().request().source());
156157

157158
if (response.getHits().getHits().length > 0) {

‎legacy/src/test/java/org/opensearch/sql/legacy/unittest/cursor/DefaultCursorTest.java‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,17 @@
88
import static org.hamcrest.MatcherAssert.assertThat;
99
import static org.hamcrest.Matchers.emptyOrNullString;
1010
import static org.hamcrest.Matchers.startsWith;
11+
import static org.junit.Assert.assertArrayEquals;
1112
import static org.junit.Assert.assertEquals;
1213
import static org.mockito.ArgumentMatchers.any;
1314
import static org.mockito.Mockito.doReturn;
1415
import static org.mockito.Mockito.when;
1516

1617
import java.io.ByteArrayOutputStream;
1718
import java.util.ArrayList;
19+
import java.util.Base64;
1820
import java.util.Collections;
21+
import org.json.JSONObject;
1922
import org.junit.Before;
2023
import org.junit.Test;
2124
import org.mockito.Mock;
@@ -88,4 +91,93 @@ public void nullCursorWhenScrollIDIsNullOrEmpty() {
8891
cursor.setScrollId("");
8992
assertThat(cursor.generateCursorId(), emptyOrNullString());
9093
}
94+
95+
@Test
96+
public void indicesAreSerializedIntoCursorId() {
97+
DefaultCursor cursor = new DefaultCursor();
98+
cursor.setRowsLeft(50);
99+
cursor.setPitId("pit-id");
100+
cursor.setIndexPattern("idx1|idx2");
101+
cursor.setFetchSize(500);
102+
cursor.setFieldAliasMap(Collections.emptyMap());
103+
cursor.setColumns(new ArrayList<>());
104+
cursor.setIndices(new String[] {"idx1", "idx2"});
105+
cursor.setSearchSourceBuilder(sourceBuilder);
106+
107+
String cursorId = cursor.generateCursorId();
108+
JSONObject decoded = decodePayload(cursorId);
109+
110+
assertEquals(2, decoded.getJSONArray("x").length());
111+
assertEquals("idx1", decoded.getJSONArray("x").getString(0));
112+
assertEquals("idx2", decoded.getJSONArray("x").getString(1));
113+
}
114+
115+
@Test
116+
public void nullIndicesAreSerializedAsEmptyArray() {
117+
DefaultCursor cursor = new DefaultCursor();
118+
cursor.setRowsLeft(50);
119+
cursor.setPitId("pit-id");
120+
cursor.setIndexPattern("idx1");
121+
cursor.setFetchSize(500);
122+
cursor.setFieldAliasMap(Collections.emptyMap());
123+
cursor.setColumns(new ArrayList<>());
124+
cursor.setSearchSourceBuilder(sourceBuilder);
125+
126+
String cursorId = cursor.generateCursorId();
127+
JSONObject decoded = decodePayload(cursorId);
128+
129+
assertEquals(0, decoded.getJSONArray("x").length());
130+
}
131+
132+
@Test
133+
public void deserializeRoundTripsIndices() {
134+
SearchSourceBuilder realSource = new SearchSourceBuilder();
135+
DefaultCursor cursor = new DefaultCursor();
136+
cursor.setRowsLeft(50);
137+
cursor.setPitId("pit-id");
138+
cursor.setIndexPattern("idx1|idx2");
139+
cursor.setFetchSize(500);
140+
cursor.setFieldAliasMap(Collections.emptyMap());
141+
cursor.setColumns(new ArrayList<>());
142+
cursor.setIndices(new String[] {"idx1", "idx2"});
143+
cursor.setSearchSourceBuilder(realSource);
144+
145+
String cursorId = cursor.generateCursorId();
146+
String payload = cursorId.substring(cursorId.indexOf(':') + 1);
147+
DefaultCursor restored = DefaultCursor.from(payload);
148+
149+
assertArrayEquals(new String[] {"idx1", "idx2"}, restored.getIndices());
150+
}
151+
152+
@Test
153+
public void deserializeLegacyCursorWithoutIndicesDefaultsToEmptyArray() {
154+
// Legacy cursor payloads written before this fix do not contain the "x" field.
155+
// They must continue to deserialize cleanly with indices == [] so in-flight
156+
// cursors from pre-fix nodes are not rejected after upgrade.
157+
SearchSourceBuilder realSource = new SearchSourceBuilder();
158+
DefaultCursor cursor = new DefaultCursor();
159+
cursor.setRowsLeft(50);
160+
cursor.setPitId("pit-id");
161+
cursor.setIndexPattern("idx1");
162+
cursor.setFetchSize(500);
163+
cursor.setFieldAliasMap(Collections.emptyMap());
164+
cursor.setColumns(new ArrayList<>());
165+
cursor.setIndices(new String[] {"idx1"});
166+
cursor.setSearchSourceBuilder(realSource);
167+
168+
String cursorId = cursor.generateCursorId();
169+
String payload = cursorId.substring(cursorId.indexOf(':') + 1);
170+
JSONObject json = new JSONObject(new String(Base64.getDecoder().decode(payload)));
171+
json.remove("x");
172+
String legacyPayload = Base64.getEncoder().encodeToString(json.toString().getBytes());
173+
174+
DefaultCursor restored = DefaultCursor.from(legacyPayload);
175+
176+
assertArrayEquals(new String[0], restored.getIndices());
177+
}
178+
179+
private JSONObject decodePayload(String cursorId) {
180+
String payload = cursorId.substring(cursorId.indexOf(':') + 1);
181+
return new JSONObject(new String(Base64.getDecoder().decode(payload)));
182+
}
91183
}

0 commit comments

Comments
 (0)