Skip to content

Commit 33b17cd

Browse files
committed
Address review comments for PR-128532
1 parent d23f275 commit 33b17cd

File tree

2 files changed

+56
-15
lines changed

2 files changed

+56
-15
lines changed

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ManageRolesPrivilegesTests.java

Lines changed: 52 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,10 @@
3434

3535
import java.io.ByteArrayOutputStream;
3636
import java.io.IOException;
37+
import java.nio.charset.StandardCharsets;
3738
import java.util.Arrays;
3839
import java.util.List;
40+
import java.util.Locale;
3941
import java.util.Objects;
4042

4143
import static org.hamcrest.Matchers.containsString;
@@ -259,28 +261,64 @@ public void testPutRoleRequestContainsNonIndexPrivileges() {
259261
}
260262

261263
public void testParseInvalidPrivilege() throws Exception {
262-
final XContent xContent = XContentType.JSON.xContent();
264+
final String unknownPrivilege = randomValueOtherThanMany(
265+
i -> IndexPrivilege.values().containsKey(i),
266+
() -> randomAlphaOfLength(10).toLowerCase(Locale.ROOT)
267+
);
263268

264-
final String invalidJsonString = """
265-
{
266-
"manage": {
267-
"indices": [
268-
{
269-
"names": ["test-*"],
270-
"privileges": ["foobar"]
271-
}
272-
]
273-
}
269+
final String invalidJsonString = String.format(Locale.ROOT, """
270+
{
271+
"manage": {
272+
"indices": [
273+
{
274+
"names": ["test-*"],
275+
"privileges": ["%s"]
276+
}
277+
]
274278
}
275-
""";
279+
}""", unknownPrivilege);
280+
assertInvalidPrivilegeParsing(invalidJsonString, unknownPrivilege);
281+
}
282+
283+
public void testParseMixedValidAndInvalidPrivileges() throws Exception {
284+
final String unknownPrivilege = randomValueOtherThanMany(
285+
i -> IndexPrivilege.values().containsKey(i),
286+
() -> randomAlphaOfLength(10).toLowerCase(Locale.ROOT)
287+
);
288+
289+
final String validPrivilege = "read";
290+
final String mixedPrivilegesJson = String.format(Locale.ROOT, """
291+
{
292+
"manage": {
293+
"indices": [
294+
{
295+
"names": ["test-*"],
296+
"privileges": ["%s", "%s"]
297+
}
298+
]
299+
}
300+
}""",validPrivilege, unknownPrivilege);
301+
302+
assertInvalidPrivilegeParsing(mixedPrivilegesJson, unknownPrivilege);
303+
}
304+
305+
/**
306+
* Helper method to assert that parsing the given JSON payload results in an
307+
* IllegalArgumentException due to an unknown privilege.
308+
*
309+
* @param jsonPayload The JSON string containing the privilege data.
310+
* @param expectedErrorDetail The specific unknown privilege name expected in the error message.
311+
*/
312+
private static void assertInvalidPrivilegeParsing(final String jsonPayload, final String expectedErrorDetail) throws Exception {
313+
final XContent xContent = XContentType.JSON.xContent();
276314

277-
try (XContentParser parser = xContent.createParser(XContentParserConfiguration.EMPTY, invalidJsonString.getBytes(StandardCharsets.UTF_8))) {
315+
try (XContentParser parser = xContent.createParser(XContentParserConfiguration.EMPTY, jsonPayload.getBytes(StandardCharsets.UTF_8))) {
278316
assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT));
279317
assertThat(parser.nextToken(), equalTo(XContentParser.Token.FIELD_NAME));
280318

281319
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> ManageRolesPrivilege.parse(parser));
282320

283-
assertThat(exception.getMessage(), containsString("unknown index privilege [foobar]"));
321+
assertThat(exception.getMessage(), containsString("unknown index privilege [" + expectedErrorDetail + "]"));
284322
}
285323
}
286324

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@
1010
import org.elasticsearch.client.Request;
1111
import org.elasticsearch.client.ResponseException;
1212
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
13+
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
1314
import org.elasticsearch.xpack.security.SecurityOnTrialLicenseRestTestCase;
1415

1516
import java.util.List;
17+
import java.util.Locale;
1618
import java.util.Map;
1719

1820
import static org.hamcrest.Matchers.contains;
@@ -21,7 +23,7 @@
2123
import static org.hamcrest.Matchers.hasSize;
2224
import static org.hamcrest.Matchers.not;
2325

24-
public class BulkPutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
26+
public class PutRoleRestIT extends SecurityOnTrialLicenseRestTestCase {
2527
public void testPutManyValidRoles() throws Exception {
2628
Map<String, Object> responseMap = upsertRoles("""
2729
{"roles": {"test1": {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["all"]}]}, "test2":
@@ -337,6 +339,7 @@ public void testPutRoleWithInvalidManageRolesPrivilege() throws Exception {
337339
}""", badRoleName)));
338340

339341
assertThat(exception.getMessage(), containsString("unknown index privilege [foobar]"));
342+
assertEquals(400, exception.getResponse().getStatusLine().getStatusCode());
340343
assertRoleDoesNotExist(badRoleName);
341344
}
342345

0 commit comments

Comments
 (0)