From faac5e3bfdd55aad69a4bb2cfc8c31c6988d46ea Mon Sep 17 00:00:00 2001 From: Andrey Pleskach Date: Tue, 7 May 2024 16:40:15 +0200 Subject: [PATCH] REST API tests refactoring (Part 4) (#4255) Signed-off-by: Andrey Pleskach --- .../api/ConfigRestApiIntegrationTest.java | 117 ++++++++++ ...DefaultApiAvailabilityIntegrationTest.java | 15 +- .../security/api/PatchPayloadHelper.java | 66 ++++++ .../rest/api/SecurityConfigApiActionTest.java | 211 ------------------ .../LegacySecurityConfigApiActionTests.java | 23 -- 5 files changed, 186 insertions(+), 246 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/api/ConfigRestApiIntegrationTest.java create mode 100644 src/integrationTest/java/org/opensearch/security/api/PatchPayloadHelper.java delete mode 100644 src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionTest.java delete mode 100644 src/test/java/org/opensearch/security/dlic/rest/api/legacy/LegacySecurityConfigApiActionTests.java diff --git a/src/integrationTest/java/org/opensearch/security/api/ConfigRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/ConfigRestApiIntegrationTest.java new file mode 100644 index 0000000000..3ccafda1d2 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/api/ConfigRestApiIntegrationTest.java @@ -0,0 +1,117 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ +package org.opensearch.security.api; + +import java.util.StringJoiner; + +import com.fasterxml.jackson.databind.node.ObjectNode; +import org.junit.Test; + +import org.opensearch.security.DefaultObjectMapper; +import org.opensearch.security.dlic.rest.api.Endpoint; +import org.opensearch.test.framework.cluster.TestRestClient; + +import static org.opensearch.security.api.PatchPayloadHelper.patch; +import static org.opensearch.security.api.PatchPayloadHelper.replaceOp; +import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.SECURITY_CONFIG_UPDATE; +import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED; +import static org.opensearch.security.support.ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION; + +public class ConfigRestApiIntegrationTest extends AbstractApiIntegrationTest { + + final static String REST_API_ADMIN_CONFIG_UPDATE = "rest-api-admin-config-update"; + + static { + clusterSettings.put(SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true).put(SECURITY_RESTAPI_ADMIN_ENABLED, true); + testSecurityConfig.withRestAdminUser(REST_ADMIN_USER, allRestAdminPermissions()) + .withRestAdminUser(REST_API_ADMIN_CONFIG_UPDATE, restAdminPermission(Endpoint.CONFIG, SECURITY_CONFIG_UPDATE)); + } + + private String securityConfigPath(final String... path) { + final var fullPath = new StringJoiner("/").add(super.apiPath("securityconfig")); + if (path != null) for (final var p : path) + fullPath.add(p); + return fullPath.toString(); + } + + @Test + public void forbiddenForRegularUsers() throws Exception { + withUser(NEW_USER, client -> { + forbidden(() -> client.get(securityConfigPath())); + forbidden(() -> client.putJson(securityConfigPath("config"), EMPTY_BODY)); + forbidden(() -> client.patch(securityConfigPath(), EMPTY_BODY)); + verifyNotAllowedMethods(client); + }); + } + + @Test + public void partiallyAvailableForAdminUser() throws Exception { + withUser(ADMIN_USER_NAME, client -> ok(() -> client.get(securityConfigPath()))); + withUser(ADMIN_USER_NAME, client -> { + badRequest(() -> client.putJson(securityConfigPath("xxx"), EMPTY_BODY)); + forbidden(() -> client.putJson(securityConfigPath("config"), EMPTY_BODY)); + forbidden(() -> client.patch(securityConfigPath(), EMPTY_BODY)); + }); + withUser(ADMIN_USER_NAME, this::verifyNotAllowedMethods); + } + + @Test + public void availableForTlsAdminUser() throws Exception { + withUser(ADMIN_USER_NAME, localCluster.getAdminCertificate(), client -> ok(() -> client.get(securityConfigPath()))); + withUser(ADMIN_USER_NAME, localCluster.getAdminCertificate(), this::verifyUpdate); + } + + @Test + public void availableForRestAdminUser() throws Exception { + withUser(REST_ADMIN_USER, client -> ok(() -> client.get(securityConfigPath()))); + withUser(REST_ADMIN_USER, this::verifyUpdate); + withUser(REST_API_ADMIN_CONFIG_UPDATE, this::verifyUpdate); + } + + void verifyUpdate(final TestRestClient client) throws Exception { + badRequest(() -> client.putJson(securityConfigPath("xxx"), EMPTY_BODY)); + verifyNotAllowedMethods(client); + + final var configJson = ok(() -> client.get(securityConfigPath())).bodyAsJsonNode(); + final var authFailureListeners = DefaultObjectMapper.objectMapper.createObjectNode(); + authFailureListeners.set( + "ip_rate_limiting", + DefaultObjectMapper.objectMapper.createObjectNode() + .put("type", "ip") + .put("allowed_tries", 10) + .put("time_window_seconds", 3_600) + .put("block_expiry_seconds", 600) + .put("max_blocked_clients", 100_000) + .put("max_tracked_clients", 100_000) + ); + authFailureListeners.set( + "internal_authentication_backend_limiting", + DefaultObjectMapper.objectMapper.createObjectNode() + .put("type", "username") + .put("authentication_backend", "intern") + .put("allowed_tries", 10) + .put("time_window_seconds", 3_600) + .put("block_expiry_seconds", 600) + .put("max_blocked_clients", 100_000) + .put("max_tracked_clients", 100_000) + ); + final var dynamicConfigJson = (ObjectNode) configJson.get("config").get("dynamic"); + dynamicConfigJson.set("auth_failure_listeners", authFailureListeners); + ok(() -> client.putJson(securityConfigPath("config"), DefaultObjectMapper.writeValueAsString(configJson.get("config"), false))); + ok(() -> client.patch(securityConfigPath(), patch(replaceOp("/config/dynamic/hosts_resolver_mode", "other")))); + } + + void verifyNotAllowedMethods(final TestRestClient client) throws Exception { + methodNotAllowed(() -> client.postJson(securityConfigPath(), EMPTY_BODY)); + methodNotAllowed(() -> client.delete(securityConfigPath())); + } + +} diff --git a/src/integrationTest/java/org/opensearch/security/api/DefaultApiAvailabilityIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/DefaultApiAvailabilityIntegrationTest.java index eaecb275db..f3b701dc38 100644 --- a/src/integrationTest/java/org/opensearch/security/api/DefaultApiAvailabilityIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/DefaultApiAvailabilityIntegrationTest.java @@ -17,6 +17,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.is; +import static org.opensearch.security.api.PatchPayloadHelper.patch; +import static org.opensearch.security.api.PatchPayloadHelper.replaceOp; public class DefaultApiAvailabilityIntegrationTest extends AbstractApiIntegrationTest { @@ -49,18 +51,7 @@ private void verifySecurityConfigApi(final TestRestClient client) throws Excepti methodNotAllowed(() -> client.putJson(apiPath("securityconfig"), EMPTY_BODY)); methodNotAllowed(() -> client.postJson(apiPath("securityconfig"), EMPTY_BODY)); methodNotAllowed(() -> client.delete(apiPath("securityconfig"))); - forbidden( - () -> client.patch( - apiPath("securityconfig"), - (builder, params) -> builder.startArray() - .startObject() - .field("op", "replace") - .field("path", "/a/b/c") - .field("value", "other") - .endObject() - .endArray() - ) - ); + forbidden(() -> client.patch(apiPath("securityconfig"), patch(replaceOp("/a/b/c", "other")))); } @Test diff --git a/src/integrationTest/java/org/opensearch/security/api/PatchPayloadHelper.java b/src/integrationTest/java/org/opensearch/security/api/PatchPayloadHelper.java new file mode 100644 index 0000000000..d7bae323bd --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/api/PatchPayloadHelper.java @@ -0,0 +1,66 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.api; + +import java.util.Locale; + +import org.opensearch.core.xcontent.ToXContentObject; + +interface PatchPayloadHelper extends ToXContentObject { + + enum Op { + ADD, + REPLACE, + REMOVE; + } + + static ToXContentObject addOp(final String path, final T value) { + return operation(Op.ADD, path, value); + } + + static ToXContentObject replaceOp(final String path, final T value) { + return operation(Op.REPLACE, path, value); + } + + static ToXContentObject removeOp(final String path) { + return operation(Op.REMOVE, path, null); + } + + private static ToXContentObject operation(final Op op, final String path, final T value) { + return (builder, params) -> { + final var opPath = path.startsWith("/") ? path : "/" + path; + builder.startObject().field("op", op.name().toLowerCase(Locale.ROOT)).field("path", opPath); + if (value != null) { + if (value instanceof ToXContentObject) { + builder.field("value", (ToXContentObject) value); + } else if (value instanceof String) { + builder.field("value", (String) value); + } else if (value instanceof Boolean) { + builder.field("value", (Boolean) value); + } else { + throw new IllegalArgumentException("Unsupported java type " + value.getClass()); + } + } + return builder.endObject(); + }; + } + + static ToXContentObject patch(final ToXContentObject... operations) { + return (builder, params) -> { + builder.startArray(); + for (final var o : operations) + o.toXContent(builder, EMPTY_PARAMS); + return builder.endArray(); + }; + } + +} diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionTest.java deleted file mode 100644 index c4066d11a2..0000000000 --- a/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionTest.java +++ /dev/null @@ -1,211 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - * - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - -package org.opensearch.security.dlic.rest.api; - -import org.apache.hc.core5.http.Header; -import org.apache.http.HttpStatus; -import org.junit.Assert; -import org.junit.Test; - -import org.opensearch.common.settings.Settings; -import org.opensearch.security.DefaultObjectMapper; -import org.opensearch.security.support.ConfigConstants; -import org.opensearch.security.test.helper.file.FileHelper; -import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; - -import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; - -public class SecurityConfigApiActionTest extends AbstractRestApiUnitTest { - private final String ENDPOINT; - - protected String getEndpointPrefix() { - return PLUGINS_PREFIX; - } - - public SecurityConfigApiActionTest() { - ENDPOINT = getEndpointPrefix() + "/api"; - } - - @Test - public void testSecurityConfigApiReadForSuperAdmin() throws Exception { - - setup(); - - rh.keystore = "restapi/kirk-keystore.jks"; - rh.sendAdminCertificate = true; - - verifyResponsesWithoutPermissionOrUnsupportedFlag(); - } - - @Test - public void testSecurityConfigApiReadRestApiUser() throws Exception { - - setupWithRestRoles(); - - rh.keystore = "restapi/kirk-keystore.jks"; - rh.sendAdminCertificate = false; - - final var restApiHeader = encodeBasicHeader("test", "test"); - verifyResponsesWithoutPermissionOrUnsupportedFlag(restApiHeader); - } - - private void verifyResponsesWithoutPermissionOrUnsupportedFlag(final Header... headers) { - HttpResponse response = rh.executeGetRequest(ENDPOINT + "/securityconfig", headers); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - - response = rh.executePutRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", headers); - Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); - - response = rh.executePostRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", headers); - Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); - - response = rh.executePatchRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", headers); - Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - - response = rh.executeDeleteRequest(ENDPOINT + "/securityconfig", headers); - Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); - } - - @Test - public void testSecurityConfigApiWriteWithUnsupportedFlagForSuperAdmin() throws Exception { - - Settings settings = Settings.builder() - .put(ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true) - .build(); - setup(settings); - - rh.keystore = "restapi/kirk-keystore.jks"; - rh.sendAdminCertificate = true; - - verifyWriteOperations(); - } - - @Test - public void testSecurityConfigApiWriteWithFullListOfPermissions() throws Exception { - - Settings settings = Settings.builder().put(ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED, true).build(); - setupWithRestRoles(settings); - - rh.keystore = "restapi/kirk-keystore.jks"; - rh.sendAdminCertificate = false; - - final var restAdminFullAccess = encodeBasicHeader("rest_api_admin_user", "rest_api_admin_user"); - verifyWriteOperations(restAdminFullAccess); - } - - @Test - public void testSecurityConfigApiWriteWithOnePermission() throws Exception { - Settings settings = Settings.builder().put(ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED, true).build(); - setupWithRestRoles(settings); - rh.keystore = "restapi/kirk-keystore.jks"; - rh.sendAdminCertificate = false; - final var updateOnlyRestApiHeader = encodeBasicHeader("rest_api_admin_config_update", "rest_api_admin_config_update"); - verifyWriteOperations(updateOnlyRestApiHeader); - } - - private void verifyWriteOperations(final Header... header) throws Exception { - HttpResponse response = rh.executeGetRequest(ENDPOINT + "/securityconfig", header); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - - response = rh.executePutRequest(ENDPOINT + "/securityconfig/xxx", FileHelper.loadFile("restapi/securityconfig.json"), header); - Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); - - response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", FileHelper.loadFile("restapi/securityconfig.json"), header); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - - response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", FileHelper.loadFile("restapi/invalid_config.json"), header); - Assert.assertEquals(HttpStatus.SC_INTERNAL_SERVER_ERROR, response.getStatusCode()); - Assert.assertTrue(response.getContentType(), response.isJsonContentType()); - Assert.assertTrue(response.getBody().contains("Unrecognized field")); - - response = rh.executeGetRequest(ENDPOINT + "/securityconfig", header); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - - response = rh.executePostRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", header); - Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); - - response = rh.executePatchRequest( - ENDPOINT + "/securityconfig", - "[{\"op\": \"replace\",\"path\": \"/config/dynamic/hosts_resolver_mode\",\"value\": \"other\"}]", - header - ); - Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); - - response = rh.executeDeleteRequest(ENDPOINT + "/securityconfig", header); - Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); - } - - @Test - public void testSecurityConfigForPatchWithUnsupportedFlag() throws Exception { - - Settings settings = Settings.builder() - .put(ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true) - .build(); - setup(settings); - - rh.keystore = "restapi/kirk-keystore.jks"; - rh.sendAdminCertificate = true; - verifyPatch(); - } - - @Test - public void testSecurityConfigForPatchWithFullPermissions() throws Exception { - - Settings settings = Settings.builder().put(ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED, true).build(); - setupWithRestRoles(settings); - - rh.keystore = "restapi/kirk-keystore.jks"; - rh.sendAdminCertificate = false; - - // non-default config - final var restAdminFullAccess = encodeBasicHeader("rest_api_admin_user", "rest_api_admin_user"); - verifyPatch(restAdminFullAccess); - } - - @Test - public void testSecurityConfigForPatchWithOnePermission() throws Exception { - - Settings settings = Settings.builder().put(ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED, true).build(); - setupWithRestRoles(settings); - - rh.keystore = "restapi/kirk-keystore.jks"; - rh.sendAdminCertificate = false; - - // non-default config - final var updateOnlyRestApiHeader = encodeBasicHeader("rest_api_admin_config_update", "rest_api_admin_config_update"); - verifyPatch(updateOnlyRestApiHeader); - } - - private void verifyPatch(final Header... header) throws Exception { - String updatedConfig = FileHelper.loadFile("restapi/securityconfig_nondefault.json"); - - // update config - HttpResponse response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", updatedConfig, header); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - - // make patch request - response = rh.executePatchRequest( - ENDPOINT + "/securityconfig", - "[{\"op\": \"add\",\"path\": \"/config/dynamic/do_not_fail_on_forbidden\",\"value\": \"false\"}]", - header - ); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - - // get config - response = rh.executeGetRequest(ENDPOINT + "/securityconfig", header); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - - // verify configs are same - Assert.assertEquals(DefaultObjectMapper.readTree(updatedConfig), DefaultObjectMapper.readTree(response.getBody()).get("config")); - } - -} diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/legacy/LegacySecurityConfigApiActionTests.java b/src/test/java/org/opensearch/security/dlic/rest/api/legacy/LegacySecurityConfigApiActionTests.java deleted file mode 100644 index 7d94de03bd..0000000000 --- a/src/test/java/org/opensearch/security/dlic/rest/api/legacy/LegacySecurityConfigApiActionTests.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - * - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - -package org.opensearch.security.dlic.rest.api.legacy; - -import org.opensearch.security.dlic.rest.api.SecurityConfigApiActionTest; - -import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; - -public class LegacySecurityConfigApiActionTests extends SecurityConfigApiActionTest { - @Override - protected String getEndpointPrefix() { - return LEGACY_OPENDISTRO_PREFIX; - } -}