Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport 2.x ] Refactor Roles REST API test and partial fix #4166 (#4433) #4478

Merged
merged 1 commit into from
Jun 20, 2024
Merged

[Backport 2.x ] Refactor Roles REST API test and partial fix #4166 (#4433) #4478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ public static void initData() {
client.prepareIndex(LIMITED_USER_INDEX).setId(ID_1).setRefreshPolicy(IMMEDIATE).setSource("foo", "bar").get();
client.prepareIndex(PROHIBITED_INDEX).setId(ID_2).setRefreshPolicy(IMMEDIATE).setSource("three", "four").get();
}
try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
Awaitility.await()
.alias("Load default configuration")
.until(() -> client.securityHealth().getTextFromJsonBody("/status"), equalTo("UP"));
}
}

@Test
Expand Down Expand Up @@ -258,7 +263,11 @@ public void testParallelTenantPutRequests() throws Exception {

AtomicInteger numCreatedResponses = new AtomicInteger();
AsyncActions.getAll(conflictingRequests, 1, TimeUnit.SECONDS).forEach((response) -> {
assertThat(response.getStatusCode(), anyOf(equalTo(HttpStatus.SC_CREATED), equalTo(HttpStatus.SC_CONFLICT)));
assertThat(
response.getBody(),
response.getStatusCode(),
anyOf(equalTo(HttpStatus.SC_CREATED), equalTo(HttpStatus.SC_CONFLICT))
);
if (response.getStatusCode() == HttpStatus.SC_CREATED) numCreatedResponses.getAndIncrement();
});
assertThat(numCreatedResponses.get(), equalTo(1)); // should only be one 201
Expand Down
34 changes: 34 additions & 0 deletions ...egrationTest/java/org/opensearch/security/api/AbstractConfigEntityApiIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

package org.opensearch.security.api;

import java.util.Map;
import java.util.Optional;
import java.util.StringJoiner;

Expand Down Expand Up @@ -79,6 +80,33 @@ public AbstractConfigEntityApiIntegrationTest(final String path, final TestDescr
this.testDescriptor = testDescriptor;
}

static ToXContentObject configJsonArray(final String... values) {
return (builder, params) -> {
builder.startArray();
if (values != null) {
for (final var v : values) {
if (v == null) {
builder.nullValue();
} else {
builder.value(v);
}
}
}
return builder.endArray();
};
}

static String[] generateArrayValues(boolean useNulls) {
final var length = randomIntBetween(1, 5);
final var values = new String[length];
final var nullIndex = randomIntBetween(0, length - 1);
for (var i = 0; i < values.length; i++) {
if (useNulls && i == nullIndex) values[i] = null;
else values[i] = randomAsciiAlphanumOfLength(10);
}
return values;
}

@Override
protected String apiPath(String... paths) {
final StringJoiner fullPath = new StringJoiner("/").add(super.apiPath(path));
Expand Down Expand Up @@ -191,6 +219,12 @@ void assertInvalidKeys(final TestRestClient.HttpResponse response, final Matcher
assertThat(response.getBody(), response.getTextFromJsonBody("/invalid_keys/keys"), expectedInvalidKeysMatcher);
}

void assertWrongDataType(final TestRestClient.HttpResponse response, final Map<String, String> expectedMessages) {
assertThat(response.getBody(), response.getTextFromJsonBody("/status"), is("error"));
for (final var p : expectedMessages.entrySet())
assertThat(response.getBody(), response.getTextFromJsonBody("/" + p.getKey()), is(p.getValue()));
}

void assertSpecifyOneOf(final TestRestClient.HttpResponse response, final String expectedSpecifyOneOfKeys) {
assertThat(response.getBody(), response.getTextFromJsonBody("/status"), is("error"));
assertThat(response.getBody(), response.getTextFromJsonBody("/reason"), equalTo("Invalid configuration"));
Expand Down
61 changes: 32 additions & 29 deletions src/integrationTest/java/org/opensearch/security/api/ActionGroupsRestApiIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
package org.opensearch.security.api;

import java.util.List;
import java.util.Map;
import java.util.Optional;

import org.opensearch.core.xcontent.ToXContentObject;
Expand Down Expand Up @@ -59,7 +60,7 @@ public ToXContentObject entityPayload(final Boolean hidden, final Boolean reserv

@Override
public ToXContentObject jsonPropertyPayload() {
return allowedActionsArray("a", "b", "c");
return configJsonArray("a", "b", "c");
}

@Override
Expand Down Expand Up @@ -89,7 +90,7 @@ static ToXContentObject actionGroup(
builder.field("type", randomType());
if (allowedActions != null) {
builder.field("allowed_actions");
allowedActionsArray(allowedActions).toXContent(builder, params);
configJsonArray(allowedActions).toXContent(builder, params);
} else {
builder.startArray("allowed_actions").endArray();
}
Expand All @@ -110,20 +111,6 @@ static String randomType() {
return randomFrom(List.of(TestSecurityConfig.ActionGroup.Type.CLUSTER.type(), TestSecurityConfig.ActionGroup.Type.INDEX.type()));
}

static ToXContentObject allowedActionsArray(final String... allowedActions) {
return (builder, params) -> {
builder.startArray();
for (final var allowedAction : allowedActions) {
if (allowedAction == null) {
builder.nullValue();
} else {
builder.value(allowedAction);
}
}
return builder.endArray();
};
}

@Override
void forbiddenToCreateEntityWithRestAdminPermissions(final TestRestClient client) throws Exception {
forbidden(() -> client.putJson(apiPath("new_rest_admin_action_group"), actionGroup(randomRestAdminPermission())));
Expand All @@ -136,10 +123,7 @@ void forbiddenToUpdateAndDeleteExistingEntityWithRestAdminPermissions(final Test
forbidden(() -> client.putJson(apiPath(REST_ADMIN_PERMISSION_ACTION_GROUP), actionGroup()));
forbidden(() -> client.patch(apiPath(), patch(replaceOp(REST_ADMIN_PERMISSION_ACTION_GROUP, actionGroup("a", "b")))));
forbidden(
() -> client.patch(
apiPath(REST_ADMIN_PERMISSION_ACTION_GROUP),
patch(replaceOp("allowed_actions", allowedActionsArray("c", "d")))
)
() -> client.patch(apiPath(REST_ADMIN_PERMISSION_ACTION_GROUP), patch(replaceOp("allowed_actions", configJsonArray("c", "d"))))
);
// remove
forbidden(() -> client.patch(apiPath(), patch(removeOp(REST_ADMIN_PERMISSION_ACTION_GROUP))));
Expand All @@ -161,7 +145,7 @@ void verifyCrudOperations(final Boolean hidden, final Boolean reserved, final Te
ok(() -> client.patch(apiPath(), patch(addOp("new_action_group_for_patch", actionGroup(hidden, reserved, "e", "f")))));
assertActionGroup(ok(() -> client.get(apiPath("new_action_group_for_patch"))), "new_action_group_for_patch", List.of("e", "f"));

ok(() -> client.patch(apiPath("new_action_group_for_patch"), patch(replaceOp("allowed_actions", allowedActionsArray("g", "h")))));
ok(() -> client.patch(apiPath("new_action_group_for_patch"), patch(replaceOp("allowed_actions", configJsonArray("g", "h")))));
assertActionGroup(ok(() -> client.get(apiPath("new_action_group_for_patch"))), "new_action_group_for_patch", List.of("g", "h"));

ok(() -> client.patch(apiPath(), patch(removeOp("new_action_group_for_patch"))));
Expand All @@ -183,32 +167,39 @@ void verifyBadRequestOperations(final TestRestClient client) throws Exception {

badRequestWithMessage(() -> client.putJson(apiPath("some_action_group"), (builder, params) -> {
builder.startObject().field("type", "asdasdsad").field("allowed_actions");
allowedActionsArray("g", "f").toXContent(builder, params);
configJsonArray("g", "f").toXContent(builder, params);
return builder.endObject();
}), "Invalid action group type: asdasdsad. Supported types are: cluster, index.");

assertMissingMandatoryKeys(
badRequest(() -> client.putJson(apiPath("some_action_group"), allowedActionsArray("a", "b", "c"))),
badRequest(() -> client.putJson(apiPath("some_action_group"), configJsonArray("a", "b", "c"))),
"allowed_actions"
);

assertMissingMandatoryKeys(
badRequest(() -> client.putJson(apiPath("some_action_group"), allowedActionsArray("a", "b", "c"))),
badRequest(() -> client.putJson(apiPath("some_action_group"), configJsonArray("a", "b", "c"))),
"allowed_actions"
);

final ToXContentObject unknownJsonFields = (builder, params) -> {
builder.startObject().field("a", "b").field("c", "d").field("allowed_actions");
allowedActionsArray("g", "h").toXContent(builder, params);
configJsonArray("g", "h").toXContent(builder, params);
return builder.endObject();
};
assertInvalidKeys(badRequest(() -> client.putJson(apiPath("some_action_group"), unknownJsonFields)), "a,c");

assertNullValuesInArray(badRequest(() -> client.putJson(apiPath("some_action_group"), (builder, params) -> {
builder.startObject().field("type", randomType()).field("allowed_actions");
allowedActionsArray("g", null, "f").toXContent(builder, params);
configJsonArray("g", null, "f").toXContent(builder, params);
return builder.endObject();
})));
assertWrongDataType(
client.putJson(
apiPath("some_action_group"),
(builder, params) -> builder.startObject().field("allowed_actions", "a").endObject()
),
Map.of("allowed_actions", "Array expected")
);
// patch
badRequest(() -> client.patch(apiPath("some_action_group"), EMPTY_BODY));
badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", EMPTY_BODY))));
Expand All @@ -224,12 +215,12 @@ void verifyBadRequestOperations(final TestRestClient client) throws Exception {
);

assertMissingMandatoryKeys(
badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", allowedActionsArray("a", "b", "c"))))),
badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", configJsonArray("a", "b", "c"))))),
"allowed_actions"
);
badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", (ToXContentObject) (builder, params) -> {
builder.startObject().field("type", "aaaa").field("allowed_actions");
allowedActionsArray("g", "f").toXContent(builder, params);
configJsonArray("g", "f").toXContent(builder, params);
return builder.endObject();
}))));

Expand All @@ -241,10 +232,22 @@ void verifyBadRequestOperations(final TestRestClient client) throws Exception {
assertNullValuesInArray(
badRequest(() -> client.patch(apiPath(), patch(addOp("some_action_group", (ToXContentObject) (builder, params) -> {
builder.startObject().field("type", randomType()).field("allowed_actions");
allowedActionsArray("g", null, "f").toXContent(builder, params);
configJsonArray("g", null, "f").toXContent(builder, params);
return builder.endObject();
}))))
);
assertWrongDataType(
client.patch(
apiPath(),
patch(
addOp(
"some_action_group",
(ToXContentObject) (builder, params) -> builder.startObject().field("allowed_actions", "a").endObject()
)
)
),
Map.of("allowed_actions", "Array expected")
);
}

void assertActionGroup(final TestRestClient.HttpResponse response, final String actionGroupName, final List<String> allowedActions) {
Expand Down
Loading
Loading