From 1449c6c4f29bf98a7093c6131522f2e48d8df6d1 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 3 Dec 2025 01:12:16 -0800 Subject: [PATCH 1/9] Hardens input validation for resource sharing APIs Signed-off-by: Darshit Chanpura --- .../securityapis/MigrateApiTests.java | 4 +- .../security/OpenSearchSecurityPlugin.java | 9 +- .../resources/ResourcePluginInfo.java | 18 + .../ResourceSharingIndexHandler.java | 6 +- .../list/AccessibleResourcesRestAction.java | 23 +- .../MigrateResourceSharingInfoApiAction.java | 93 ++-- .../resources/api/share/ShareRequest.java | 33 +- .../resources/api/share/ShareRestAction.java | 12 +- .../resources/sharing/Recipients.java | 23 +- .../resources/sharing/ResourceSharing.java | 4 +- .../security/resources/sharing/ShareWith.java | 6 +- .../resources/utils/InputValidation.java | 186 ++++++++ ...t.java => ResourceAccessHandlerTests.java} | 2 +- .../resources/sharing/ShareWithTests.java | 10 +- .../resources/utils/InputValidationTests.java | 406 ++++++++++++++++++ 15 files changed, 776 insertions(+), 59 deletions(-) create mode 100644 src/main/java/org/opensearch/security/resources/utils/InputValidation.java rename src/test/java/org/opensearch/security/resources/{ResourceAccessHandlerTest.java => ResourceAccessHandlerTests.java} (99%) create mode 100644 src/test/java/org/opensearch/security/resources/utils/InputValidationTests.java diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java index 58ac9e2b2c..c25560ef78 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java @@ -401,7 +401,9 @@ public void testMigrateAPIWithSuperAdmin_invalidDefaultAccessLevel() { migrateResponse, RestMatchers.isBadRequest( "/message", - "Invalid access level blah for resource sharing for resource type [" + RESOURCE_TYPE + "]" + "Invalid access level blah for resource type [" + + RESOURCE_TYPE + + "]. Allowed: sample_read_write, sample_read_only, sample_full_access" ) ); } diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 9802e95680..57ba1c58d2 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -708,7 +708,14 @@ public List getRestHandlers( new ShareRestAction(resourcePluginInfo, resourceSharingEnabledSetting, resourceSharingProtectedResourceTypesSetting) ); handlers.add(new ResourceTypesRestAction(resourcePluginInfo, resourceSharingEnabledSetting)); - handlers.add(new AccessibleResourcesRestAction(resourceAccessHandler, resourcePluginInfo, resourceSharingEnabledSetting)); + handlers.add( + new AccessibleResourcesRestAction( + resourceAccessHandler, + resourcePluginInfo, + resourceSharingEnabledSetting, + resourceSharingProtectedResourceTypesSetting + ) + ); } log.debug("Added {} rest handler(s)", handlers.size()); diff --git a/src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java b/src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java index 4ace9be889..5c3deeae6a 100644 --- a/src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java +++ b/src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java @@ -295,4 +295,22 @@ public Set getResourceIndicesForProtectedTypes() { } } + public List currentProtectedTypes() { + lock.readLock().lock(); + try { + return protectedTypesSetting.getDynamicSettingValue(); + } finally { + lock.readLock().unlock(); + } + } + + public Set availableAccessLevels() { + lock.readLock().lock(); + try { + return typeToAccessLevels.values().stream().flatMap(Set::stream).collect(Collectors.toCollection(LinkedHashSet::new)); + } finally { + lock.readLock().unlock(); + } + } + } diff --git a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java index 77ea71eafc..a80bf05b1a 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java @@ -579,7 +579,7 @@ public void fetchSharingInfo(String resourceIndex, String resourceId, ActionList .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, getResponse.getSourceAsString()) ) { parser.nextToken(); - ResourceSharing resourceSharing = ResourceSharing.fromXContent(parser); + ResourceSharing resourceSharing = ResourceSharing.fromXContent(parser, resourcePluginInfo.availableAccessLevels()); resourceSharing.setResourceId(getResponse.getId()); LOGGER.debug( @@ -995,7 +995,7 @@ public void fetchAccessibleResourceSharingRecords( ) ) { p.nextToken(); - ResourceSharing rs = ResourceSharing.fromXContent(p); + ResourceSharing rs = ResourceSharing.fromXContent(p, resourcePluginInfo.availableAccessLevels()); boolean canShare = canUserShare(user, /* isAdmin */ false, rs, resourceType); out.add(new SharingRecord(rs, canShare)); } catch (Exception ex) { @@ -1104,7 +1104,7 @@ private void processScrollResultsAndCollectSharingRecords( ) ) { parser.nextToken(); - ResourceSharing rs = ResourceSharing.fromXContent(parser); + ResourceSharing rs = ResourceSharing.fromXContent(parser, resourcePluginInfo.availableAccessLevels()); boolean canShare = canUserShare(user, isAdmin, rs, resourceType); resourceSharingRecords.add(new SharingRecord(rs, canShare)); } catch (Exception e) { diff --git a/src/main/java/org/opensearch/security/resources/api/list/AccessibleResourcesRestAction.java b/src/main/java/org/opensearch/security/resources/api/list/AccessibleResourcesRestAction.java index 0337b095a5..d19619555c 100644 --- a/src/main/java/org/opensearch/security/resources/api/list/AccessibleResourcesRestAction.java +++ b/src/main/java/org/opensearch/security/resources/api/list/AccessibleResourcesRestAction.java @@ -10,7 +10,6 @@ import java.io.IOException; import java.util.List; -import java.util.Objects; import java.util.Set; import com.google.common.collect.ImmutableList; @@ -46,16 +45,19 @@ public class AccessibleResourcesRestAction extends BaseRestHandler { private final ResourceAccessHandler resourceAccessHandler; private final ResourcePluginInfo resourcePluginInfo; private final OpensearchDynamicSetting resourceSharingEnabledSetting; + private final OpensearchDynamicSetting> resourceSharingProtectedTypesSetting; public AccessibleResourcesRestAction( final ResourceAccessHandler resourceAccessHandler, ResourcePluginInfo resourcePluginInfo, - OpensearchDynamicSetting resourceSharingEnabledSetting + OpensearchDynamicSetting resourceSharingEnabledSetting, + OpensearchDynamicSetting> resourceSharingProtectedTypesSetting ) { super(); this.resourceAccessHandler = resourceAccessHandler; this.resourcePluginInfo = resourcePluginInfo; this.resourceSharingEnabledSetting = resourceSharingEnabledSetting; + this.resourceSharingProtectedTypesSetting = resourceSharingProtectedTypesSetting; } @Override @@ -73,19 +75,30 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli if (!resourceSharingEnabledSetting.getDynamicSettingValue()) { return channel -> { channel.sendResponse(new BytesRestResponse(RestStatus.NOT_IMPLEMENTED, "Feature disabled.")); }; } - final String resourceType = Objects.requireNonNull(request.param("resource_type"), "resource_type is required"); + final String resourceType = request.param("resource_type"); + + if (resourceType == null) { + return channel -> { channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, "resource_type is required")); }; + } final String resourceIndex = resourcePluginInfo.indexByType(resourceType); if (resourceIndex == null) { - return channel -> { handleResponse(channel, Set.of()); }; + return channel -> { + String message = String.format( + "Invalid resource type: %s. Must be one of: %s", + resourceType, + resourceSharingProtectedTypesSetting.getDynamicSettingValue() + ); + channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, message)); + }; } return channel -> resourceAccessHandler.getResourceSharingInfoForCurrentUser(resourceType, ActionListener.wrap(rows -> { handleResponse(channel, rows); }, e -> handleError(channel, e))); } - private void handleResponse(RestChannel channel, Set records) throws IOException { + private void handleResponse(RestChannel channel, Set records) { try (XContentBuilder b = channel.newBuilder()) { b.startObject(); b.startArray("resources"); diff --git a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java index 5884de5c8a..231db3697b 100644 --- a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java +++ b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java @@ -10,6 +10,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -59,6 +60,7 @@ import org.opensearch.security.resources.sharing.Recipients; import org.opensearch.security.resources.sharing.ResourceSharing; import org.opensearch.security.resources.sharing.ShareWith; +import org.opensearch.security.resources.utils.InputValidation; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.spi.resources.ResourceProvider; import org.opensearch.threadpool.ThreadPool; @@ -153,44 +155,73 @@ private void handleMigrate(RestChannel channel, RestRequest request, Client clie private ValidationResult loadCurrentSharingInfo(RestRequest request, Client client) throws IOException { JsonNode body = Utils.toJsonNode(request.content().utf8ToString()); - String sourceIndex = body.get("source_index").asText(); - String userNamePath = body.get("username_path").asText(); - String backendRolesPath = body.get("backend_roles_path").asText(); - String defaultOwner = body.get("default_owner").asText(); - JsonNode node = body.get("default_access_level"); - Map typeToDefaultAccessLevel = Utils.toMapOfStrings(node); - if (!resourcePluginInfo.getResourceIndicesForProtectedTypes().contains(sourceIndex)) { - String badRequestMessage = "Invalid resource index " + sourceIndex + "."; - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(badRequestMessage)); + // Required fields + String sourceIndex = InputValidation.getRequiredText(body, "source_index", InputValidation.MAX_INDEX_NAME_LENGTH); + String userNamePath = InputValidation.getRequiredText(body, "username_path", InputValidation.MAX_PATH_LENGTH); + String backendRolesPath = InputValidation.getRequiredText(body, "backend_roles_path", InputValidation.MAX_PATH_LENGTH); + + // Optional + String defaultOwner = InputValidation.getOptionalText(body, "default_owner", InputValidation.MAX_PRINCIPAL_LENGTH); + + // Raw JSON for default_access_level + JsonNode defaultAccessNode = body.get("default_access_level"); + + // Validate JSON structure for default_access_level (object, non-empty, values non-empty) + try { + InputValidation.validateDefaultAccessLevelNode(defaultAccessNode); + } catch (IllegalArgumentException e) { + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(e.getMessage())); + } + + // Validate paths & owner early + InputValidation.validateJsonPath("username_path", userNamePath); + InputValidation.validateJsonPath("backend_roles_path", backendRolesPath); + InputValidation.validateDefaultOwner(defaultOwner); + + // Validate source index + try { + InputValidation.validateSourceIndex(sourceIndex, resourcePluginInfo.getResourceIndicesForProtectedTypes()); + } catch (Exception e) { + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(e.getMessage())); } + // Convert after structural validation + Map typeToDefaultAccessLevel = defaultAccessNode == null || defaultAccessNode.isNull() + ? Collections.emptyMap() + : Utils.toMapOfStrings(defaultAccessNode); + String typePath = null; - for (String type : typeToDefaultAccessLevel.keySet()) { + + // Validate each type + its accessLevel + for (Map.Entry entry : typeToDefaultAccessLevel.entrySet()) { + String type = entry.getKey(); + String defaultAccessLevelForType = entry.getValue(); + + // Validate resource type exists ResourceProvider provider = resourcePluginInfo.getResourceProvider(type); - String defaultAccessLevelForType = typeToDefaultAccessLevel.get(type); - LOGGER.info("Default access level for resource type [{}] is [{}]", type, typeToDefaultAccessLevel.get(type)); - // check that access level exists for given resource-index if (provider == null) { - String badRequestMessage = "Invalid resource type " + type + "."; - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(badRequestMessage)); + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Invalid resource type " + type + ".")); } - typePath = provider.typeField(); // All types in the same index must have same typeField - var accessLevels = resourcePluginInfo.flattenedForType(type).actionGroups(); - if (!accessLevels.contains(defaultAccessLevelForType)) { - LOGGER.error( - "Invalid access level {} for resource sharing for resource type [{}]. Available access-levels are [{}]", - defaultAccessLevelForType, - type, - accessLevels + + typePath = provider.typeField(); + + // Allowed access-levels for this type + Set accessLevels = resourcePluginInfo.flattenedForType(type).actionGroups(); + + try { + InputValidation.validateAccessLevel(defaultAccessLevelForType, accessLevels); + } catch (Exception e) { + return ValidationResult.error( + RestStatus.BAD_REQUEST, + badRequestMessage( + "Invalid access level " + + defaultAccessLevelForType + + " for resource type [" + + type + + "]. Allowed: " + + String.join(", ", accessLevels) + ) ); - String badRequestMessage = "Invalid access level " - + defaultAccessLevelForType - + " for resource sharing for resource type [" - + type - + "]. Available access-levels are [" - + accessLevels - + "]"; - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(badRequestMessage)); } } diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java index a6cc04cb52..77480730a5 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java @@ -9,6 +9,8 @@ package org.opensearch.security.resources.api.share; import java.io.IOException; +import java.util.List; +import java.util.Set; import com.fasterxml.jackson.annotation.JsonProperty; @@ -22,6 +24,7 @@ import org.opensearch.rest.RestRequest; import org.opensearch.security.resources.ResourcePluginInfo; import org.opensearch.security.resources.sharing.ShareWith; +import org.opensearch.security.resources.utils.InputValidation; /** * This class represents a request to share access to a resource. @@ -188,6 +191,9 @@ public ShareRequest build() { } public void parseContent(XContentParser xContentParser, ResourcePluginInfo resourcePluginInfo) throws IOException { + final List allowedTypes = resourcePluginInfo.currentProtectedTypes(); + final Set validAccessLevels = resourcePluginInfo.availableAccessLevels(); + try (XContentParser parser = xContentParser) { XContentParser.Token token; // START_OBJECT while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { @@ -196,20 +202,35 @@ public void parseContent(XContentParser xContentParser, ResourcePluginInfo resou parser.nextToken(); switch (name) { case "resource_id": - this.resourceId(parser.text()); + String resourceId = parser.text(); + InputValidation.validateResourceId(resourceId); + this.resourceId(resourceId); break; case "resource_type": - this.resourceType(parser.text()); - this.resourceIndex(resourcePluginInfo.indexByType(parser.text())); + String resourceType = parser.text(); + + // Validate type syntax + membership in dynamic protected-types list + InputValidation.validateResourceType(resourceType, allowedTypes); + + // Resolve to index, using pluginInfo + String indexName = resourcePluginInfo.indexByType(resourceType); + if (indexName == null) { + throw new IllegalArgumentException( + "Invalid resource_type [" + resourceType + "]. Allowed types: " + String.join(", ", allowedTypes) + ); + } + + this.resourceType(resourceType); + this.resourceIndex(resourcePluginInfo.indexByType(resourceType)); break; case "share_with": - this.shareWith(ShareWith.fromXContent(parser)); + this.shareWith(ShareWith.fromXContent(parser, validAccessLevels)); break; case "add": - this.add(ShareWith.fromXContent(parser)); + this.add(ShareWith.fromXContent(parser, validAccessLevels)); break; case "revoke": - this.revoke(ShareWith.fromXContent(parser)); + this.revoke(ShareWith.fromXContent(parser, validAccessLevels)); break; default: parser.skipChildren(); diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java b/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java index 7213d80c62..1ac1f6b916 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java @@ -16,6 +16,7 @@ import org.apache.logging.log4j.Logger; import org.opensearch.core.rest.RestStatus; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestRequest; @@ -85,7 +86,16 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli builder.resourceType(resourceType); if (request.hasContent()) { - builder.parseContent(request.contentParser(), resourcePluginInfo); + try (XContentParser parser = request.contentParser()) { + builder.parseContent(parser, resourcePluginInfo); + } catch (IllegalArgumentException e) { + return channel -> { + channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, "Invalid request content. " + e.getMessage())); + }; + } catch (IllegalStateException e) { + return channel -> { channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, e.getMessage())); }; + } + } if (builder.resourceId == null || builder.resourceType == null) { diff --git a/src/main/java/org/opensearch/security/resources/sharing/Recipients.java b/src/main/java/org/opensearch/security/resources/sharing/Recipients.java index 759e8d65ac..ff0c89a72a 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/Recipients.java +++ b/src/main/java/org/opensearch/security/resources/sharing/Recipients.java @@ -22,6 +22,7 @@ import org.opensearch.core.xcontent.ToXContentFragment; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.security.resources.utils.InputValidation; /** * This class represents the entities with which a resource is shared for a particular action-group. @@ -103,12 +104,30 @@ public static Recipients fromXContent(XContentParser parser) throws IOException while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { String fieldName = parser.currentName(); - Recipient recipient = Recipient.valueOf(fieldName.toUpperCase(Locale.ROOT)); + + final Recipient recipient; + try { + recipient = Recipient.valueOf(fieldName.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Unknown recipient type [" + fieldName + "]", e); + } parser.nextToken(); + if (parser.currentToken() != XContentParser.Token.START_ARRAY) { + throw new IllegalArgumentException("Expected array for [" + fieldName + "], but found [" + parser.currentToken() + "]"); + } + Set values = new HashSet<>(); + int count = 0; + while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - values.add(parser.text()); + count++; + InputValidation.validateArrayEntryCount(fieldName, count); + + String value = parser.text(); + InputValidation.validatePrincipalValue(fieldName, value); + + values.add(value); } recipients.put(recipient, values); } diff --git a/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java b/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java index 4e58d784f5..a29c12e035 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java @@ -195,7 +195,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder.endObject(); } - public static ResourceSharing fromXContent(XContentParser parser) throws IOException { + public static ResourceSharing fromXContent(XContentParser parser, Set validAccessLevels) throws IOException { Builder b = ResourceSharing.builder(); String currentFieldName = null; @@ -220,7 +220,7 @@ public static ResourceSharing fromXContent(XContentParser parser) throws IOExcep b.createdBy(CreatedBy.fromXContent(parser)); break; case "share_with": - b.shareWith(ShareWith.fromXContent(parser)); + b.shareWith(ShareWith.fromXContent(parser, validAccessLevels)); break; default: parser.skipChildren(); diff --git a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java index 84a3ccbfc8..75285c102a 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java @@ -22,6 +22,7 @@ import org.opensearch.core.xcontent.ToXContentFragment; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.security.resources.utils.InputValidation; /** * This class contains information about whom a resource is shared with and what is the action-group associated with it. @@ -96,7 +97,7 @@ public XContentBuilder toXContent(XContentBuilder b, Params params) throws IOExc return b.endObject(); } - public static ShareWith fromXContent(XContentParser parser) throws IOException { + public static ShareWith fromXContent(XContentParser parser, Set validAccessLevels) throws IOException { Map sharingInfo = new HashMap<>(); if (parser.currentToken() != XContentParser.Token.START_OBJECT) { @@ -109,6 +110,9 @@ public static ShareWith fromXContent(XContentParser parser) throws IOException { if (token == XContentParser.Token.FIELD_NAME) { String accessLevel = parser.currentName(); + // We don't expect access-levels for a type to be null unless the type doesn't exist + InputValidation.validateAccessLevel(accessLevel, validAccessLevels); + parser.nextToken(); Recipients recipients = Recipients.fromXContent(parser); diff --git a/src/main/java/org/opensearch/security/resources/utils/InputValidation.java b/src/main/java/org/opensearch/security/resources/utils/InputValidation.java new file mode 100644 index 0000000000..e0e92b2180 --- /dev/null +++ b/src/main/java/org/opensearch/security/resources/utils/InputValidation.java @@ -0,0 +1,186 @@ +/* + * 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. + */ + +package org.opensearch.security.resources.utils; + +import java.util.List; +import java.util.Set; +import java.util.regex.Pattern; + +import com.fasterxml.jackson.databind.JsonNode; + +import org.opensearch.core.common.Strings; + +/** + * Validates user inputs supplied to resource-sharing REST APIs + */ +public final class InputValidation { + + private InputValidation() {} + + public static final int MAX_RESOURCE_ID_LENGTH = 256; + public static final int MAX_RESOURCE_TYPE_LENGTH = 256; + public static final int MAX_ACCESS_LEVEL_LENGTH = 256; + public static final int MAX_PRINCIPAL_LENGTH = 256; + public static final int MAX_PATH_LENGTH = 256; + public static final int MAX_INDEX_NAME_LENGTH = 256; + public static final int MAX_ARRAY_SIZE = 100_000; + + // Alphanumeric + _ - : + private static final Pattern SAFE_VALUE = Pattern.compile("^[A-Za-z0-9_:-]+$");; + + /* ---------------------- generic helpers ---------------------- */ + + public static void requireNonEmpty(String fieldName, String value) { + if (Strings.isNullOrEmpty(value)) { + throw new IllegalArgumentException(fieldName + " must not be null or empty"); + } + } + + public static void validateMaxLength(String fieldName, String value, int maxLength) { + if (value.length() > maxLength) { + throw new IllegalArgumentException(fieldName + " length [" + value.length() + "] exceeds max [" + maxLength + "]"); + } + } + + public static void validateSafeValue(String fieldName, String value, int maxLength) { + requireNonEmpty(fieldName, value); + validateMaxLength(fieldName, value, maxLength); + if (!SAFE_VALUE.matcher(value).matches()) { + throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); + } + } + + public static void validateArrayEntryCount(String fieldName, int count) { + if (count > MAX_ARRAY_SIZE) { + throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); + } + } + + public static void validateResourceId(String resourceId) { + validateSafeValue("resource_id", resourceId, MAX_RESOURCE_ID_LENGTH); + } + + public static void validateResourceType(String resourceType, List allowedTypes) { + validateSafeValue("resource_type", resourceType, MAX_RESOURCE_TYPE_LENGTH); + + if (allowedTypes == null || allowedTypes.isEmpty()) { + throw new IllegalStateException("No protected resource types configured"); + } + + if (!allowedTypes.contains(resourceType)) { + throw new IllegalArgumentException("Unsupported resource_type [" + resourceType + "], allowed types: " + allowedTypes); + } + } + + public static void validatePrincipalValue(String fieldName, String value) { + // users / roles / backend_roles entries + validateSafeValue(fieldName, value, MAX_PRINCIPAL_LENGTH); + } + + public static void validateAccessLevel(String accessLevel, Set validAccessLevels) { + requireNonEmpty("access_level", accessLevel); + + validateMaxLength("access_level", accessLevel, MAX_ACCESS_LEVEL_LENGTH); + + if (!SAFE_VALUE.matcher(accessLevel).matches()) { + throw new IllegalArgumentException("Invalid access_level [" + accessLevel + "]. Allowed characters: A-Z a-z 0-9 _ - :"); + } + + // Check against configured access-level set + if (validAccessLevels == null || validAccessLevels.isEmpty()) { + throw new IllegalStateException("No access levels configured."); + } + + if (!validAccessLevels.contains(accessLevel)) { + throw new IllegalArgumentException( + "Invalid access_level [" + accessLevel + "]. Allowed values: " + String.join(", ", validAccessLevels) + ); + } + } + + /* -------- JSON helpers for migrate API -------- */ + + public static String getRequiredText(JsonNode body, String fieldName, int maxLength) { + JsonNode node = body.get(fieldName); + if (node == null || node.isNull() || !node.isTextual()) { + throw new IllegalArgumentException("Field [" + fieldName + "] is required and must be a non-empty string"); + } + String value = node.asText(); + requireNonEmpty(fieldName, value); + validateMaxLength(fieldName, value, maxLength); + return value; + } + + public static String getOptionalText(JsonNode body, String fieldName, int maxLength) { + JsonNode node = body.get(fieldName); + if (node == null || node.isNull()) { + return null; + } + if (!node.isTextual()) { + throw new IllegalArgumentException("Field [" + fieldName + "] must be a string when provided"); + } + String value = node.asText(); + if (value.isEmpty()) { + return null; + } + validateMaxLength(fieldName, value, maxLength); + return value; + } + + /* --------- migrate-specific primitives --------- */ + + public static void validateJsonPath(String fieldName, String path) { + requireNonEmpty(fieldName, path); + validateMaxLength(fieldName, path, MAX_PATH_LENGTH); + // simple rule: no whitespace anywhere + if (!path.equals(path.trim()) || path.chars().anyMatch(Character::isWhitespace)) { + throw new IllegalArgumentException(fieldName + " must not contain whitespace"); + } + } + + public static void validateSourceIndex(String sourceIndex, Set allowedIndices) { + requireNonEmpty("source_index", sourceIndex); + validateMaxLength("source_index", sourceIndex, MAX_INDEX_NAME_LENGTH); + if (allowedIndices == null || allowedIndices.isEmpty()) { + throw new IllegalStateException("No protected resource indices configured"); + } + if (!allowedIndices.contains(sourceIndex)) { + throw new IllegalArgumentException("Invalid resource index [" + sourceIndex + "]. Allowed indices: " + allowedIndices); + } + } + + public static void validateDefaultOwner(String defaultOwner) { + if (defaultOwner == null) { + return; // optional + } + validatePrincipalValue("default_owner", defaultOwner); + } + + public static void validateDefaultAccessLevelNode(JsonNode node) { + if (node == null || node.isNull()) { + return; // field is optional + } + + if (!node.isObject()) { + throw new IllegalArgumentException("default_access_level must be an object"); + } + + if (!node.fieldNames().hasNext()) { + throw new IllegalArgumentException("default_access_level cannot be empty"); + } + + // Validate values are non-empty strings + node.fields().forEachRemaining(entry -> { + JsonNode val = entry.getValue(); + if (!val.isTextual() || val.asText().isEmpty()) { + throw new IllegalArgumentException("default_access_level for type [" + entry.getKey() + "] must be a non-empty string"); + } + }); + } +} diff --git a/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTest.java b/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java similarity index 99% rename from src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTest.java rename to src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java index c1b44c31f9..8fdf5dd584 100644 --- a/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTest.java +++ b/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java @@ -42,7 +42,7 @@ @RunWith(MockitoJUnitRunner.class) @SuppressWarnings("unchecked") // action listener mock -public class ResourceAccessHandlerTest { +public class ResourceAccessHandlerTests { @Mock private ThreadPool threadPool; diff --git a/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java b/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java index f300a5f38a..f0614108ec 100644 --- a/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java +++ b/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java @@ -58,7 +58,7 @@ public void testFromXContentWhenCurrentTokenIsNotStartObject() throws IOExceptio parser.nextToken(); - ShareWith shareWith = ShareWith.fromXContent(parser); + ShareWith shareWith = ShareWith.fromXContent(parser, Set.of("read_only")); assertThat(shareWith, notNullValue()); Recipients readOnly = shareWith.atAccessLevel("read_only"); @@ -77,7 +77,7 @@ public void testFromXContentWithEmptyInput() throws IOException { String emptyJson = "{}"; XContentParser parser = XContentType.JSON.xContent().createParser(NamedXContentRegistry.EMPTY, null, emptyJson); - ShareWith result = ShareWith.fromXContent(parser); + ShareWith result = ShareWith.fromXContent(parser, Set.of()); assertThat(result, notNullValue()); assertThat(result.isPrivate(), is(true)); @@ -106,7 +106,7 @@ public void testFromXContentWithStartObject() throws IOException { parser.nextToken(); - ShareWith shareWith = ShareWith.fromXContent(parser); + ShareWith shareWith = ShareWith.fromXContent(parser, Set.of("read_only", "default")); assertThat(shareWith, notNullValue()); @@ -132,7 +132,7 @@ public void testFromXContentWithUnexpectedEndOfInput() throws IOException { when(mockParser.currentToken()).thenReturn(XContentParser.Token.START_OBJECT); when(mockParser.nextToken()).thenReturn(XContentParser.Token.END_OBJECT, (XContentParser.Token) null); - ShareWith result = ShareWith.fromXContent(mockParser); + ShareWith result = ShareWith.fromXContent(mockParser, Set.of()); assertThat(result, notNullValue()); assertThat(result.isPrivate(), is(true)); @@ -202,7 +202,7 @@ public void test_fromXContent_emptyObject() throws IOException { parser = XContentType.JSON.xContent().createParser(null, null, builder.toString()); } - ShareWith shareWith = ShareWith.fromXContent(parser); + ShareWith shareWith = ShareWith.fromXContent(parser, Set.of()); assertThat(shareWith.isPrivate(), is(true)); assertThat(shareWith.isPublic(), is(false)); diff --git a/src/test/java/org/opensearch/security/resources/utils/InputValidationTests.java b/src/test/java/org/opensearch/security/resources/utils/InputValidationTests.java new file mode 100644 index 0000000000..8a88d731ac --- /dev/null +++ b/src/test/java/org/opensearch/security/resources/utils/InputValidationTests.java @@ -0,0 +1,406 @@ +/* + * 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. + */ + +package org.opensearch.security.resources.utils; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +public class InputValidationTests { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + /* ---------------------- helpers ---------------------- */ + + private static String repeat(char c, int count) { + char[] arr = new char[count]; + Arrays.fill(arr, c); + return new String(arr); + } + + private static void expectThrows(Class expected, Runnable runnable) { + try { + runnable.run(); + fail("Expected exception of type " + expected.getName()); + } catch (Throwable t) { + assertTrue( + "Unexpected exception type. Expected " + expected.getName() + " but got " + t.getClass().getName(), + expected.isInstance(t) + ); + @SuppressWarnings("unchecked") + T cast = (T) t; + } + } + + private static JsonNode json(String s) throws Exception { + return MAPPER.readTree(s); + } + + /* ---------------------- requireNonEmpty ---------------------- */ + + @Test + public void testRequireNonEmptyAcceptsNonEmpty() { + InputValidation.requireNonEmpty("field", "value"); + } + + @Test + public void testRequireNonEmptyRejectsNull() { + expectThrows(IllegalArgumentException.class, () -> InputValidation.requireNonEmpty("field", null)); + } + + @Test + public void testRequireNonEmptyRejectsEmpty() { + expectThrows(IllegalArgumentException.class, () -> InputValidation.requireNonEmpty("field", "")); + } + + /* ---------------------- validateMaxLength ---------------------- */ + + @Test + public void testValidateMaxLengthAtBoundary() { + String value = repeat('a', 5); + InputValidation.validateMaxLength("field", value, 5); + } + + @Test + public void testValidateMaxLengthRejectsOverLimit() { + String value = repeat('a', 6); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateMaxLength("field", value, 5)); + } + + /* ---------------------- validateSafeValue ---------------------- */ + + @Test + public void testValidateSafeValueAcceptsAllowedCharacters() { + String value = "Abc123_-:xyz"; + InputValidation.validateSafeValue("field", value, 50); + } + + @Test + public void testValidateSafeValueRejectsInvalidCharacters() { + String value = "bad value$"; // space + $ + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateSafeValue("field", value, 50)); + } + + @Test + public void testValidateSafeValueRejectsTooLong() { + String value = repeat('a', 11); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateSafeValue("field", value, 10)); + } + + /* ---------------------- validateArrayEntryCount ---------------------- */ + + @Test + public void testValidateArrayEntryCountAtMaxBoundary() { + InputValidation.validateArrayEntryCount("field", InputValidation.MAX_ARRAY_SIZE); + } + + @Test + public void testValidateArrayEntryCountRejectsAboveMax() { + int overMax = InputValidation.MAX_ARRAY_SIZE + 1; + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateArrayEntryCount("field", overMax)); + } + + /* ---------------------- validateResourceId ---------------------- */ + + @Test + public void testValidateResourceIdAcceptsValidId() { + String id = "resource_123-ABC:xyz"; + InputValidation.validateResourceId(id); + } + + @Test + public void testValidateResourceIdRejectsInvalidCharacters() { + String id = "invalid id!"; // space + ! + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateResourceId(id)); + } + + @Test + public void testValidateResourceIdRejectsTooLong() { + String id = repeat('a', InputValidation.MAX_RESOURCE_ID_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateResourceId(id)); + } + + /* ---------------------- validateResourceType ---------------------- */ + + @Test + public void testValidateResourceTypeAcceptsValidTypeInAllowedList() { + List allowedTypes = Arrays.asList("anomaly-detector", "forecaster", "ml-model"); + InputValidation.validateResourceType("anomaly-detector", allowedTypes); + } + + @Test + public void testValidateResourceTypeRejectsInvalidCharacters() { + List allowedTypes = List.of("anomaly-detector"); + String resourceType = "anomaly detector"; // contains space + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateResourceType(resourceType, allowedTypes)); + } + + @Test + public void testValidateResourceTypeRejectsWhenNoAllowedTypesConfiguredNull() { + expectThrows(IllegalStateException.class, () -> InputValidation.validateResourceType("anomaly-detector", null)); + } + + @Test + public void testValidateResourceTypeRejectsWhenNoAllowedTypesConfiguredEmpty() { + expectThrows(IllegalStateException.class, () -> InputValidation.validateResourceType("anomaly-detector", Collections.emptyList())); + } + + @Test + public void testValidateResourceTypeRejectsWhenTypeNotInAllowedList() { + List allowedTypes = Arrays.asList("anomaly-detector", "forecaster"); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateResourceType("ml-model", allowedTypes)); + } + + /* ---------------------- validatePrincipalValue ---------------------- */ + + @Test + public void testValidatePrincipalValueAcceptsValidPrincipal() { + InputValidation.validatePrincipalValue("users", "user_123-role:1"); + } + + @Test + public void testValidatePrincipalValueRejectsInvalidCharacters() { + expectThrows( + IllegalArgumentException.class, + () -> InputValidation.validatePrincipalValue("users", "user name") // space + ); + } + + @Test + public void testValidatePrincipalValueRejectsTooLong() { + String principal = repeat('u', InputValidation.MAX_PRINCIPAL_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validatePrincipalValue("users", principal)); + } + + /* ---------------------- validateAccessLevel ---------------------- */ + + @Test + public void testValidateAccessLevelAcceptsValidValueInSet() { + Set allowed = new HashSet<>(Arrays.asList("rd_read_only", "rd_write", "forecast:read", "forecast:write")); + + InputValidation.validateAccessLevel("rd_read_only", allowed); + InputValidation.validateAccessLevel("forecast:write", allowed); + } + + @Test + public void testValidateAccessLevelRejectsNullAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel(null, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsEmptyAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel("", allowed)); + } + + @Test + public void testValidateAccessLevelRejectsTooLongAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + String longAccess = repeat('a', InputValidation.MAX_ACCESS_LEVEL_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel(longAccess, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsInvalidCharacters() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + String invalid = "rd read"; // space + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel(invalid, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsWhenNoAccessLevelsConfiguredNull() { + expectThrows(IllegalStateException.class, () -> InputValidation.validateAccessLevel("rd_read_only", null)); + } + + @Test + public void testValidateAccessLevelRejectsWhenNoAccessLevelsConfiguredEmpty() { + expectThrows(IllegalStateException.class, () -> InputValidation.validateAccessLevel("rd_read_only", Collections.emptySet())); + } + + @Test + public void testValidateAccessLevelRejectsWhenNotInAllowedSet() { + Set allowed = new HashSet<>(Arrays.asList("rd_read_only", "rd_write")); + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel("forecast:read", allowed)); + } + + @Test + public void testGetRequiredTextReturnsValueWhenPresent() throws Exception { + JsonNode body = json("{\"source_index\":\"index-1\"}"); + + String result = InputValidation.getRequiredText(body, "source_index", InputValidation.MAX_INDEX_NAME_LENGTH); + + assertEquals("index-1", result); + } + + @Test + public void testGetRequiredTextThrowsWhenMissing() throws Exception { + JsonNode body = json("{\"other\":\"value\"}"); + + expectThrows( + IllegalArgumentException.class, + () -> InputValidation.getRequiredText(body, "source_index", InputValidation.MAX_INDEX_NAME_LENGTH) + ); + } + + @Test + public void testGetRequiredTextThrowsWhenNonTextual() throws Exception { + JsonNode body = json("{\"source_index\":123}"); + + expectThrows( + IllegalArgumentException.class, + () -> InputValidation.getRequiredText(body, "source_index", InputValidation.MAX_INDEX_NAME_LENGTH) + ); + } + + /* ---------------------- getOptionalText ---------------------- */ + + @Test + public void testGetOptionalTextReturnsNullWhenMissing() throws Exception { + JsonNode body = json("{\"other\":\"value\"}"); + + String result = InputValidation.getOptionalText(body, "default_owner", InputValidation.MAX_PRINCIPAL_LENGTH); + + assertNull(result); + } + + @Test + public void testGetOptionalTextReturnsValueWhenPresent() throws Exception { + JsonNode body = json("{\"default_owner\":\"owner_1\"}"); + + String result = InputValidation.getOptionalText(body, "default_owner", InputValidation.MAX_PRINCIPAL_LENGTH); + + assertEquals("owner_1", result); + } + + @Test + public void testGetOptionalTextThrowsWhenNonTextual() throws Exception { + JsonNode body = json("{\"default_owner\":123}"); + + expectThrows( + IllegalArgumentException.class, + () -> InputValidation.getOptionalText(body, "default_owner", InputValidation.MAX_PRINCIPAL_LENGTH) + ); + } + + /* ---------------------- validateJsonPath ---------------------- */ + + @Test + public void testValidateJsonPathAcceptsValidPath() { + InputValidation.validateJsonPath("username_path", "user.details.name"); + } + + @Test + public void testValidateJsonPathRejectsEmpty() { + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateJsonPath("username_path", "")); + } + + @Test + public void testValidateJsonPathRejectsWhitespace() { + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateJsonPath("username_path", " user . name ")); + } + + /* ---------------------- validateSourceIndex ---------------------- */ + + @Test + public void testValidateSourceIndexAcceptsWhenInAllowedSet() { + Set allowed = new HashSet<>(); + allowed.add("index-1"); + allowed.add("index-2"); + + InputValidation.validateSourceIndex("index-1", allowed); + } + + @Test + public void testValidateSourceIndexRejectsWhenNotInAllowedSet() { + Set allowed = new HashSet<>(); + allowed.add("index-1"); + + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateSourceIndex("index-2", allowed)); + } + + @Test + public void testValidateSourceIndexRejectsWhenNoIndicesConfiguredNull() { + expectThrows(IllegalStateException.class, () -> InputValidation.validateSourceIndex("index-1", null)); + } + + @Test + public void testValidateSourceIndexRejectsWhenNoIndicesConfiguredEmpty() { + expectThrows(IllegalStateException.class, () -> InputValidation.validateSourceIndex("index-1", Collections.emptySet())); + } + + /* ---------------------- validateDefaultOwner ---------------------- */ + + @Test + public void testValidateDefaultOwnerAllowsNull() { + // optional field + InputValidation.validateDefaultOwner(null); + } + + @Test + public void testValidateDefaultOwnerAcceptsValidPrincipal() { + InputValidation.validateDefaultOwner("owner_123-role:1"); + } + + @Test + public void testValidateDefaultOwnerRejectsInvalidCharacters() { + expectThrows( + IllegalArgumentException.class, + () -> InputValidation.validateDefaultOwner("owner name") // space + ); + } + + /* ---------------------- validateDefaultAccessLevelNode ---------------------- */ + + @Test + public void testValidateDefaultAccessLevelNodeAllowsNull() { + // field absent / null is allowed (optional) + InputValidation.validateDefaultAccessLevelNode(null); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsNonObject() throws Exception { + JsonNode node = json("\"string-not-object\""); + + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsEmptyObject() throws Exception { + JsonNode node = json("{}"); + + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsEmptyValue() throws Exception { + JsonNode node = json("{\"anomaly-detector\":\"\"}"); + + expectThrows(IllegalArgumentException.class, () -> InputValidation.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeAcceptsNonEmptyValues() throws Exception { + JsonNode node = json("{ \"anomaly-detector\": \"rd_read_only\", \"forecaster\": \"rd_write\" }"); + + // should not throw + InputValidation.validateDefaultAccessLevelNode(node); + } +} From 42d778c1e7d665d12e6308536ecfd26087a1ed4b Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 3 Dec 2025 01:40:15 -0800 Subject: [PATCH 2/9] Fixes existing tests Signed-off-by: Darshit Chanpura --- .../securityapis/AccessibleResourcesApiTests.java | 9 +++++---- .../security/resources/utils/InputValidation.java | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/AccessibleResourcesApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/AccessibleResourcesApiTests.java index fe4d2fd402..fcb4868248 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/AccessibleResourcesApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/AccessibleResourcesApiTests.java @@ -74,13 +74,14 @@ public void clearIndices() { } @Test - @SuppressWarnings("unchecked") public void testListAccessibleResources_gibberishParams() { try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { TestRestClient.HttpResponse response = client.get(SECURITY_LIST_ENDPOINT + "?resource_type=" + "some-type"); - response.assertStatusCode(HttpStatus.SC_OK); - List types = (List) response.bodyAsMap().get("resources"); - assertThat(types.size(), equalTo(0)); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat( + response.getBody(), + containsString("Invalid resource type: some-type. Must be one of: [sample-resource, sample-resource-group]") + ); } } diff --git a/src/main/java/org/opensearch/security/resources/utils/InputValidation.java b/src/main/java/org/opensearch/security/resources/utils/InputValidation.java index e0e92b2180..802cd84007 100644 --- a/src/main/java/org/opensearch/security/resources/utils/InputValidation.java +++ b/src/main/java/org/opensearch/security/resources/utils/InputValidation.java @@ -31,8 +31,8 @@ private InputValidation() {} public static final int MAX_INDEX_NAME_LENGTH = 256; public static final int MAX_ARRAY_SIZE = 100_000; - // Alphanumeric + _ - : - private static final Pattern SAFE_VALUE = Pattern.compile("^[A-Za-z0-9_:-]+$");; + // Alphanumeric + _ - : OR : * - "*" is only allowed as standalone + private static final Pattern SAFE_VALUE = Pattern.compile("^(\\*|[A-Za-z0-9_:-]+)$"); /* ---------------------- generic helpers ---------------------- */ From 50044ccf8956c50cdcfbd64c2b080e4cbf7bda5e Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 3 Dec 2025 02:43:52 -0800 Subject: [PATCH 3/9] Adds extra tests for input validation Signed-off-by: Darshit Chanpura --- .../securityapis/MigrateApiTests.java | 176 ++++++++++++++++++ .../resource/securityapis/ShareApiTests.java | 102 +++++++++- .../resources/api/share/ShareRestAction.java | 6 +- 3 files changed, 276 insertions(+), 8 deletions(-) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java index c25560ef78..fea697c2b6 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java @@ -409,6 +409,182 @@ public void testMigrateAPIWithSuperAdmin_invalidDefaultAccessLevel() { } } + @Test + public void testMigrateAPI_inputValidation_invalidValues() { + // Ensure there is at least one resource so migration can proceed to validation + createSampleResource(); + + try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { + + // ------------------------------ + // 1) Invalid username_path (whitespace) + // ------------------------------ + String invalidUserPathPayload = """ + { + "source_index": "%s", + "username_path": " /user", + "backend_roles_path": "/user/backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "sample_read_only" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + TestRestClient.HttpResponse response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidUserPathPayload); + + assertThat(response, RestMatchers.isBadRequest("/error/reason", "username_path must not contain whitespace")); + + // ------------------------------ + // 2) Invalid backend_roles_path (whitespace) + // ------------------------------ + String invalidBackendPathPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by. backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "sample_read_only" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidBackendPathPayload); + + assertThat(response, RestMatchers.isBadRequest("/error/reason", "backend_roles_path must not contain whitespace")); + + // ------------------------------ + // 3) Invalid default_owner (bad characters) + // ------------------------------ + String invalidDefaultOwnerPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "owner name", + "default_access_level": { + "%s": "sample_read_only" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidDefaultOwnerPayload); + + assertThat( + response, + RestMatchers.isBadRequest("/error/reason", "default_owner contains invalid characters; allowed: A-Z a-z 0-9 _ - :") + ); + + // ------------------------------ + // 4) default_access_level is NOT an object + // ------------------------------ + String defaultAccessNotObjectPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": "sample_read_only" + } + """.formatted(RESOURCE_INDEX_NAME); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, defaultAccessNotObjectPayload); + + assertThat(response, RestMatchers.isBadRequest("/reason", "Wrong datatype")); + + // ------------------------------ + // 5) default_access_level is an empty object {} + // ------------------------------ + String defaultAccessEmptyObjectPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": { } + } + """.formatted(RESOURCE_INDEX_NAME); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, defaultAccessEmptyObjectPayload); + + assertThat(response, RestMatchers.isBadRequest("/message", "default_access_level cannot be empty")); + + // ------------------------------ + // 6) default_access_level has empty value for a type + // ------------------------------ + String defaultAccessEmptyValuePayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, defaultAccessEmptyValuePayload); + + assertThat( + response, + RestMatchers.isBadRequest("/message", "default_access_level for type [" + RESOURCE_TYPE + "] must be a non-empty string") + ); + + // ------------------------------ + // 7) Invalid source_index (not in protected types) + // ------------------------------ + String invalidSourceIndexPayload = """ + { + "source_index": "some-other-index", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "sample_read_only" + } + } + """.formatted(RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidSourceIndexPayload); + + assertThat( + response, + RestMatchers.isBadRequest( + "/message", + "Invalid resource index [some-other-index]. Allowed indices: [" + RESOURCE_INDEX_NAME + "]" + ) + ); + + // ------------------------------ + // 8) Invalid access level value for valid type + // (this exercises validateAccessLevel + last try/catch) + // ------------------------------ + String invalidAccessLevelPayload = """ + { + "source_index": "%s", + "username_path": "created_by.user", + "backend_roles_path": "created_by.backend_roles", + "default_owner": "some_user", + "default_access_level": { + "%s": "blah" + } + } + """.formatted(RESOURCE_INDEX_NAME, RESOURCE_TYPE); + + response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidAccessLevelPayload); + + assertThat( + response, + RestMatchers.isBadRequest( + "/message", + "Invalid access level blah for resource type [sample-resource]. Allowed: sample_read_write, sample_read_only, sample_full_access" + ) + ); + } + } + private String createSampleResource() { try (TestRestClient client = cluster.getRestClient(MIGRATION_USER)) { String sampleResource = """ diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java index f4260ffade..ba1d059ec8 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java @@ -95,7 +95,7 @@ public void testGibberishPayload() { SECURITY_SHARE_ENDPOINT + "?resource_id=" + "some-id" + "&resource_type=" + RESOURCE_TYPE ); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); // since resource-index exists but resource-id doesn't, but user - // shouldn't know that + // shouldn't know that response = client.get(SECURITY_SHARE_ENDPOINT + "?resource_id=" + adminResId + "&resource_type=" + "some-type"); response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); // since type doesn't exist, so does the corresponding index @@ -108,7 +108,7 @@ public void testGibberishPayload() { putSharingInfoPayload("some-id", RESOURCE_TYPE, SAMPLE_READ_ONLY, Recipient.USERS, NO_ACCESS_USER.getName()) ); response.assertStatusCode(HttpStatus.SC_FORBIDDEN); // since resource-index exists but resource-id doesn't, but user - // shouldn't know that + // shouldn't know that response = client.putJson( SECURITY_SHARE_ENDPOINT, @@ -342,6 +342,102 @@ public void testPostSharingInfo() { response.assertStatusCode(HttpStatus.SC_FORBIDDEN); } } - } + @Test + public void testShareApi_putAndPatch_invalidPayloadValidation() { + // Use admin so we actually hit payload parsing & validation, not auth failures + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + + // 1) PUT with invalid resource_id (invalid chars) + String putInvalidResourceIdPayload = """ + { + "resource_id": "invalid id", // space not allowed + "resource_type": "%s", + "share_with": { + "%s": { + "users": ["%s"] + } + } + } + """.formatted(RESOURCE_TYPE, SAMPLE_READ_ONLY, NO_ACCESS_USER.getName()); + + TestRestClient.HttpResponse response = client.putJson(SECURITY_SHARE_ENDPOINT, putInvalidResourceIdPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("resource_id")); + assertThat(response.getBody(), containsString("contains invalid characters")); + + // 2) PUT with invalid principal value (users entry contains space) + String putInvalidUserPayload = """ + { + "resource_id": "%s", + "resource_type": "%s", + "share_with": { + "%s": { + "users": ["invalid user"] + } + } + } + """.formatted(adminResId, RESOURCE_TYPE, SAMPLE_READ_ONLY); + + response = client.putJson(SECURITY_SHARE_ENDPOINT, putInvalidUserPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("users")); + assertThat(response.getBody(), containsString("contains invalid characters")); + + // 3) PUT with invalid access level key + String putInvalidAccessLevelPayload = """ + { + "resource_id": "%s", + "resource_type": "%s", + "share_with": { + "blah": { + "users": ["%s"] + } + } + } + """.formatted(adminResId, RESOURCE_TYPE, NO_ACCESS_USER.getName()); + + response = client.putJson(SECURITY_SHARE_ENDPOINT, putInvalidAccessLevelPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("Invalid access_level")); + assertThat(response.getBody(), containsString("blah")); + + // 4) PATCH with invalid principal value (same validation path as PUT, via Recipients) + String patchInvalidUserPayload = """ + { + "resource_id": "%s", + "resource_type": "%s", + "add": { + "%s": { + "users": ["invalid user"] + } + } + } + """.formatted(adminResId, RESOURCE_TYPE, SAMPLE_READ_ONLY); + + response = client.patch(SECURITY_SHARE_ENDPOINT, patchInvalidUserPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("users")); + assertThat(response.getBody(), containsString("contains invalid characters")); + + // 5) PATCH with invalid access level key + String patchInvalidAccessLevelPayload = """ + { + "resource_id": "%s", + "resource_type": "%s", + "add": { + "blah": { + "users": ["%s"] + } + } + } + """.formatted(adminResId, RESOURCE_TYPE, NO_ACCESS_USER.getName()); + + response = client.patch(SECURITY_SHARE_ENDPOINT, patchInvalidAccessLevelPayload); + response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); + assertThat(response.getBody(), containsString("Invalid access_level")); + assertThat(response.getBody(), containsString("blah")); + } + } + } } diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java b/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java index 1ac1f6b916..b627e2a539 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareRestAction.java @@ -88,11 +88,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli if (request.hasContent()) { try (XContentParser parser = request.contentParser()) { builder.parseContent(parser, resourcePluginInfo); - } catch (IllegalArgumentException e) { - return channel -> { - channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, "Invalid request content. " + e.getMessage())); - }; - } catch (IllegalStateException e) { + } catch (IllegalArgumentException | IllegalStateException e) { return channel -> { channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, e.getMessage())); }; } From 1e06d7617917b3634c5653612cb84dd15c4979b6 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 3 Dec 2025 02:46:07 -0800 Subject: [PATCH 4/9] force commons-logging to 1.3.5 Signed-off-by: Darshit Chanpura --- build.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/build.gradle b/build.gradle index e8bef057c7..af4c132818 100644 --- a/build.gradle +++ b/build.gradle @@ -479,6 +479,7 @@ configurations { all { resolutionStrategy { force 'commons-codec:commons-codec:1.20.0' + force 'commons-logging:commons-logging:1.3.5' force 'org.slf4j:slf4j-api:1.7.36' force 'org.scala-lang:scala-library:2.13.18' force "com.fasterxml.jackson:jackson-bom:${versions.jackson}" From e424ec385b8096bc0cac519eaee8c09718a1e48e Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 3 Dec 2025 02:48:17 -0800 Subject: [PATCH 5/9] Adds changelog entry Signed-off-by: Darshit Chanpura --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f32bacb66..256c278524 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add support for X509 v3 extensions (SAN) for authentication ([#5701](https://github.com/opensearch-project/security/pull/5701)) - [Resource Sharing] Requires default_owner for resource/migrate API ([#5789](https://github.com/opensearch-project/security/pull/5789)) - Add --timeout (-to) as an option to securityadmin.sh ([#5787](https://github.com/opensearch-project/security/pull/5787)) +- Hardens input validation for resource sharing APIs ([#5831](https://github.com/opensearch-project/security/pull/5831) ### Bug Fixes - Create a WildcardMatcher.NONE when creating a WildcardMatcher with an empty string ([#5694](https://github.com/opensearch-project/security/pull/5694)) From dc750e3b3677a2bcfe4aedf687677017d2d01943 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 3 Dec 2025 13:28:41 -0800 Subject: [PATCH 6/9] Fixes typo and force commons-logging Signed-off-by: Darshit Chanpura --- sample-resource-plugin/build.gradle | 1 + .../opensearch/security/resources/sharing/ShareWithTests.java | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sample-resource-plugin/build.gradle b/sample-resource-plugin/build.gradle index 28c0f1b2bf..8401d1a524 100644 --- a/sample-resource-plugin/build.gradle +++ b/sample-resource-plugin/build.gradle @@ -70,6 +70,7 @@ configurations { force 'org.apache.httpcomponents:httpclient:4.5.14' force 'org.apache.httpcomponents:httpcore:4.4.16' force 'commons-codec:commons-codec:1.20.0' + force 'commons-logging:commons-logging:1.3.5' force 'org.hamcrest:hamcrest:2.2' force 'org.mockito:mockito-core:5.20.0' force 'org.slf4j:slf4j-api:1.7.36' diff --git a/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java b/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java index f0614108ec..d06a6484db 100644 --- a/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java +++ b/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java @@ -106,7 +106,7 @@ public void testFromXContentWithStartObject() throws IOException { parser.nextToken(); - ShareWith shareWith = ShareWith.fromXContent(parser, Set.of("read_only", "default")); + ShareWith shareWith = ShareWith.fromXContent(parser, Set.of("read-only", "default")); assertThat(shareWith, notNullValue()); From c526cfb4d3c868e7acf0dcd313e72fad30e31999 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 4 Dec 2025 15:57:35 -0800 Subject: [PATCH 7/9] Refactors to request content validator Signed-off-by: Darshit Chanpura --- .../resource/securityapis/ShareApiTests.java | 8 +- .../validation/RequestContentValidator.java | 431 +++++++++++++++++- .../ResourceSharingIndexHandler.java | 6 +- .../MigrateResourceSharingInfoApiAction.java | 112 +++-- .../resources/api/share/ShareRequest.java | 18 +- .../resources/sharing/Recipients.java | 26 +- .../resources/sharing/ResourceSharing.java | 4 +- .../security/resources/sharing/ShareWith.java | 29 +- .../resources/utils/InputValidation.java | 186 -------- .../RequestContentValidatorTest.java | 386 +++++++++++++++- .../resources/sharing/ShareWithTests.java | 19 +- .../resources/utils/InputValidationTests.java | 406 ----------------- 12 files changed, 975 insertions(+), 656 deletions(-) delete mode 100644 src/main/java/org/opensearch/security/resources/utils/InputValidation.java delete mode 100644 src/test/java/org/opensearch/security/resources/utils/InputValidationTests.java diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java index ba1d059ec8..96a9002173 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/ShareApiTests.java @@ -399,8 +399,8 @@ public void testShareApi_putAndPatch_invalidPayloadValidation() { response = client.putJson(SECURITY_SHARE_ENDPOINT, putInvalidAccessLevelPayload); response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); - assertThat(response.getBody(), containsString("Invalid access_level")); - assertThat(response.getBody(), containsString("blah")); + assertThat(response.getBody(), containsString("access_level must be one of:")); + assertThat(response.getBody(), not(containsString("blah"))); // 4) PATCH with invalid principal value (same validation path as PUT, via Recipients) String patchInvalidUserPayload = """ @@ -435,8 +435,8 @@ public void testShareApi_putAndPatch_invalidPayloadValidation() { response = client.patch(SECURITY_SHARE_ENDPOINT, patchInvalidAccessLevelPayload); response.assertStatusCode(HttpStatus.SC_BAD_REQUEST); - assertThat(response.getBody(), containsString("Invalid access_level")); - assertThat(response.getBody(), containsString("blah")); + assertThat(response.getBody(), containsString("access_level must be one of:")); + assertThat(response.getBody(), not(containsString("blah"))); } } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java index f3056250df..ed9ad829ec 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java @@ -85,6 +85,78 @@ public static enum DataType { BOOLEAN; } + /** + * Validator interface for field-level validation, similar to OpenSearch Setting.Validator + */ + @FunctionalInterface + public interface FieldValidator { + /** + * Validate a field value + * @param fieldName the name of the field being validated + * @param value the value to validate (can be String, JsonNode, etc.) + * @throws IllegalArgumentException if validation fails + */ + void validate(String fieldName, Object value); + } + + /** + * Configuration for a field including its type and validation rules + */ + public static class FieldConfiguration { + private final DataType dataType; + private final Integer maxLength; + private final FieldValidator validator; + + private FieldConfiguration(DataType dataType, Integer maxLength, FieldValidator validator) { + this.dataType = dataType; + this.maxLength = maxLength; + this.validator = validator; + } + + public static FieldConfiguration of(DataType dataType) { + return new FieldConfiguration(dataType, null, null); + } + + public static FieldConfiguration of(DataType dataType, int maxLength) { + return new FieldConfiguration(dataType, maxLength, null); + } + + public static FieldConfiguration of(DataType dataType, FieldValidator validator) { + return new FieldConfiguration(dataType, null, validator); + } + + public static FieldConfiguration of(DataType dataType, int maxLength, FieldValidator validator) { + return new FieldConfiguration(dataType, maxLength, validator); + } + + public DataType getDataType() { + return dataType; + } + + public Integer getMaxLength() { + return maxLength; + } + + public FieldValidator getValidator() { + return validator; + } + + public void validate(String fieldName, Object value) { + // Validate max length for strings + if (maxLength != null && value instanceof String) { + String strValue = (String) value; + if (strValue.length() > maxLength) { + throw new IllegalArgumentException(fieldName + " length [" + strValue.length() + "] exceeds max [" + maxLength + "]"); + } + } + + // Run custom validator if present + if (validator != null) { + validator.validate(fieldName, value); + } + } + } + public interface ValidationContext { default boolean hasParams() { @@ -105,6 +177,15 @@ default Set mandatoryOrKeys() { Map allowedKeys(); + /** + * Optional: Returns field configurations with validation rules. + * If not overridden, returns null and the validator will use allowedKeys() instead. + * This is an opt-in enhancement for more flexible validation. + */ + default Map allowedKeysWithConfig() { + return null; + } + } protected ValidationError validationError; @@ -177,7 +258,14 @@ protected ValidationResult validateJsonKeys(final JsonNode jsonContent mandatory.removeAll(requestedKeys); missingMandatoryKeys.addAll(mandatory); - final Set allowed = new HashSet<>(validationContext.allowedKeys().keySet()); + // Use allowedKeysWithConfig if provided, otherwise fall back to allowedKeys + final Map fieldConfigs = validationContext.allowedKeysWithConfig(); + final Set allowed; + if (fieldConfigs != null) { + allowed = new HashSet<>(fieldConfigs.keySet()); + } else { + allowed = new HashSet<>(validationContext.allowedKeys().keySet()); + } requestedKeys.removeAll(allowed); invalidKeys.addAll(requestedKeys); @@ -189,14 +277,32 @@ protected ValidationResult validateJsonKeys(final JsonNode jsonContent } private ValidationResult validateDataType(final JsonNode jsonContent) { + // Check if enhanced validation is available + final Map fieldConfigs = validationContext.allowedKeysWithConfig(); + final boolean useEnhancedValidation = (fieldConfigs != null); + try (final JsonParser parser = DefaultObjectMapper.objectMapper.treeAsTokens(jsonContent)) { JsonToken token; while ((token = parser.nextToken()) != null) { if (token.equals(JsonToken.FIELD_NAME)) { String currentName = parser.getCurrentName(); - final DataType dataType = validationContext.allowedKeys().get(currentName); + + // Get data type from either FieldConfiguration or simple DataType map + final DataType dataType; + final FieldConfiguration fieldConfig; + + if (useEnhancedValidation && fieldConfigs != null) { + fieldConfig = fieldConfigs.get(currentName); + dataType = (fieldConfig != null) ? fieldConfig.getDataType() : null; + } else { + fieldConfig = null; + dataType = validationContext.allowedKeys().get(currentName); + } + if (dataType != null) { JsonToken valueToken = parser.nextToken(); + + // Validate data type switch (dataType) { case INTEGER: if (valueToken != JsonToken.VALUE_NUMBER_INT) { @@ -206,16 +312,45 @@ private ValidationResult validateDataType(final JsonNode jsonContent) case STRING: if (valueToken != JsonToken.VALUE_STRING) { wrongDataTypes.put(currentName, "String expected"); + } else if (fieldConfig != null) { + // Enhanced validation: validate string-specific constraints + String stringValue = parser.getText(); + try { + fieldConfig.validate(currentName, stringValue); + } catch (IllegalArgumentException e) { + wrongDataTypes.put(currentName, e.getMessage()); + } } break; case ARRAY: if (valueToken != JsonToken.START_ARRAY && valueToken != JsonToken.END_ARRAY) { wrongDataTypes.put(currentName, "Array expected"); + } else if (fieldConfig != null && fieldConfig.getValidator() != null) { + // Enhanced validation: validate array content + JsonNode arrayNode = jsonContent.get(currentName); + try { + fieldConfig.validate(currentName, arrayNode); + } catch (IllegalArgumentException e) { + wrongDataTypes.put(currentName, e.getMessage()); + } } break; case OBJECT: if (!valueToken.equals(JsonToken.START_OBJECT) && !valueToken.equals(JsonToken.END_OBJECT)) { wrongDataTypes.put(currentName, "Object expected"); + } else if (fieldConfig != null && fieldConfig.getValidator() != null) { + // Enhanced validation: validate object content + JsonNode objectNode = jsonContent.get(currentName); + try { + fieldConfig.validate(currentName, objectNode); + } catch (IllegalArgumentException e) { + wrongDataTypes.put(currentName, e.getMessage()); + } + } + break; + case BOOLEAN: + if (valueToken != JsonToken.VALUE_TRUE && valueToken != JsonToken.VALUE_FALSE) { + wrongDataTypes.put(currentName, "Boolean expected"); } break; } @@ -393,4 +528,296 @@ public String message() { } } + + /* ======================================================================== + * Input Validation Utilities (for resource-sharing REST APIs) + * ======================================================================== */ + + public static final int MAX_RESOURCE_ID_LENGTH = 256; + public static final int MAX_RESOURCE_TYPE_LENGTH = 256; + public static final int MAX_ACCESS_LEVEL_LENGTH = 256; + public static final int MAX_PRINCIPAL_LENGTH = 256; + public static final int MAX_PATH_LENGTH = 256; + public static final int MAX_INDEX_NAME_LENGTH = 256; + public static final int MAX_ARRAY_SIZE = 100_000; + + // Alphanumeric + _ - : OR : * - "*" is only allowed as standalone + private static final java.util.regex.Pattern SAFE_VALUE = java.util.regex.Pattern.compile("^(\\*|[A-Za-z0-9_:-]+)$"); + + /* ---------------------- generic helpers ---------------------- */ + + public static void requireNonEmpty(String fieldName, String value) { + if (Strings.isNullOrEmpty(value)) { + throw new IllegalArgumentException(fieldName + " must not be null or empty"); + } + } + + public static void validateMaxLength(String fieldName, String value, int maxLength) { + if (value.length() > maxLength) { + throw new IllegalArgumentException(fieldName + " length [" + value.length() + "] exceeds max [" + maxLength + "]"); + } + } + + public static void validateSafeValue(String fieldName, String value, int maxLength) { + requireNonEmpty(fieldName, value); + validateMaxLength(fieldName, value, maxLength); + if (!SAFE_VALUE.matcher(value).matches()) { + throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); + } + } + + public static void validateArrayEntryCount(String fieldName, int count) { + if (count > MAX_ARRAY_SIZE) { + throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); + } + } + + public static void validateResourceId(String resourceId) { + validateSafeValue("resource_id", resourceId, MAX_RESOURCE_ID_LENGTH); + } + + public static void validateResourceType(String resourceType, java.util.List allowedTypes) { + validateSafeValue("resource_type", resourceType, MAX_RESOURCE_TYPE_LENGTH); + + if (allowedTypes == null || allowedTypes.isEmpty()) { + throw new IllegalStateException("No protected resource types configured"); + } + + if (!allowedTypes.contains(resourceType)) { + throw new IllegalArgumentException("Unsupported resource_type [" + resourceType + "], allowed types: " + allowedTypes); + } + } + + public static void validatePrincipalValue(String fieldName, String value) { + // users / roles / backend_roles entries + validateSafeValue(fieldName, value, MAX_PRINCIPAL_LENGTH); + } + + public static void validateAccessLevel(String accessLevel, Set validAccessLevels) { + requireNonEmpty("access_level", accessLevel); + + validateMaxLength("access_level", accessLevel, MAX_ACCESS_LEVEL_LENGTH); + + if (!SAFE_VALUE.matcher(accessLevel).matches()) { + throw new IllegalArgumentException("Invalid access_level [" + accessLevel + "]. Allowed characters: A-Z a-z 0-9 _ - :"); + } + + // Check against configured access-level set + if (validAccessLevels == null || validAccessLevels.isEmpty()) { + throw new IllegalStateException("No access levels configured."); + } + + if (!validAccessLevels.contains(accessLevel)) { + throw new IllegalArgumentException( + "Invalid access_level [" + accessLevel + "]. Allowed values: " + String.join(", ", validAccessLevels) + ); + } + } + + /* -------- JSON helpers for migrate API -------- */ + + public static String getRequiredText(JsonNode body, String fieldName, int maxLength) { + JsonNode node = body.get(fieldName); + if (node == null || node.isNull() || !node.isTextual()) { + throw new IllegalArgumentException("Field [" + fieldName + "] is required and must be a non-empty string"); + } + String value = node.asText(); + requireNonEmpty(fieldName, value); + validateMaxLength(fieldName, value, maxLength); + return value; + } + + public static String getOptionalText(JsonNode body, String fieldName, int maxLength) { + JsonNode node = body.get(fieldName); + if (node == null || node.isNull()) { + return null; + } + if (!node.isTextual()) { + throw new IllegalArgumentException("Field [" + fieldName + "] must be a string when provided"); + } + String value = node.asText(); + if (value.isEmpty()) { + return null; + } + validateMaxLength(fieldName, value, maxLength); + return value; + } + + /* --------- migrate-specific primitives --------- */ + + public static void validateJsonPath(String fieldName, String path) { + requireNonEmpty(fieldName, path); + validateMaxLength(fieldName, path, MAX_PATH_LENGTH); + // simple rule: no whitespace anywhere + if (!path.equals(path.trim()) || path.chars().anyMatch(Character::isWhitespace)) { + throw new IllegalArgumentException(fieldName + " must not contain whitespace"); + } + } + + public static void validateSourceIndex(String sourceIndex, Set allowedIndices) { + requireNonEmpty("source_index", sourceIndex); + validateMaxLength("source_index", sourceIndex, MAX_INDEX_NAME_LENGTH); + if (allowedIndices == null || allowedIndices.isEmpty()) { + throw new IllegalStateException("No protected resource indices configured"); + } + if (!allowedIndices.contains(sourceIndex)) { + throw new IllegalArgumentException("Invalid resource index [" + sourceIndex + "]. Allowed indices: " + allowedIndices); + } + } + + public static void validateDefaultOwner(String defaultOwner) { + if (defaultOwner == null) { + return; // optional + } + validatePrincipalValue("default_owner", defaultOwner); + } + + public static void validateDefaultAccessLevelNode(JsonNode node) { + if (node == null || node.isNull()) { + return; // field is optional + } + + if (!node.isObject()) { + throw new IllegalArgumentException("default_access_level must be an object"); + } + + if (!node.fieldNames().hasNext()) { + throw new IllegalArgumentException("default_access_level cannot be empty"); + } + + // Validate values are non-empty strings + node.fields().forEachRemaining(entry -> { + JsonNode val = entry.getValue(); + if (!val.isTextual() || val.asText().isEmpty()) { + throw new IllegalArgumentException("default_access_level for type [" + entry.getKey() + "] must be a non-empty string"); + } + }); + } + + /* ======================================================================== + * Pre-built Field Validators for Common Use Cases + * ======================================================================== */ + + /** + * Validator that ensures a string contains only safe characters (alphanumeric + _ - :) + * and optionally allows standalone "*" + */ + public static final FieldValidator SAFE_VALUE_VALIDATOR = (fieldName, value) -> { + if (value instanceof String) { + String strValue = (String) value; + requireNonEmpty(fieldName, strValue); + if (!SAFE_VALUE.matcher(strValue).matches()) { + throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); + } + } + }; + + /** + * Validator for resource IDs + */ + public static final FieldValidator RESOURCE_ID_VALIDATOR = (fieldName, value) -> { + if (value instanceof String) { + String strValue = (String) value; + requireNonEmpty(fieldName, strValue); + validateMaxLength(fieldName, strValue, MAX_RESOURCE_ID_LENGTH); + if (!SAFE_VALUE.matcher(strValue).matches()) { + throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); + } + } + }; + + /** + * Validator for principal values (users, roles, backend_roles) + */ + public static final FieldValidator PRINCIPAL_VALIDATOR = (fieldName, value) -> { + if (value instanceof String) { + String strValue = (String) value; + requireNonEmpty(fieldName, strValue); + validateMaxLength(fieldName, strValue, MAX_PRINCIPAL_LENGTH); + if (!SAFE_VALUE.matcher(strValue).matches()) { + throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); + } + } + }; + + /** + * Validator for JSON paths (no whitespace allowed) + */ + public static final FieldValidator JSON_PATH_VALIDATOR = (fieldName, value) -> { + if (value instanceof String) { + String strValue = (String) value; + requireNonEmpty(fieldName, strValue); + validateMaxLength(fieldName, strValue, MAX_PATH_LENGTH); + if (!strValue.equals(strValue.trim()) || strValue.chars().anyMatch(Character::isWhitespace)) { + throw new IllegalArgumentException(fieldName + " must not contain whitespace"); + } + } + }; + + /** + * Validator for array entry counts (works with JsonNode arrays) + */ + public static final FieldValidator ARRAY_SIZE_VALIDATOR = (fieldName, value) -> { + if (value instanceof JsonNode) { + JsonNode node = (JsonNode) value; + if (node.isArray() && node.size() > MAX_ARRAY_SIZE) { + throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); + } + } else if (value instanceof Integer) { + int count = (Integer) value; + if (count > MAX_ARRAY_SIZE) { + throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); + } + } + }; + + /** + * Validator that ensures array elements are non-null and non-blank + */ + public static final FieldValidator NON_EMPTY_ARRAY_ELEMENTS_VALIDATOR = (fieldName, value) -> { + if (value instanceof JsonNode) { + JsonNode node = (JsonNode) value; + if (node.isArray()) { + for (JsonNode element : node) { + if (element.isNull() || (element.isTextual() && Strings.isNullOrEmpty(element.asText().trim()))) { + throw new IllegalArgumentException(fieldName + " contains null or blank array elements"); + } + } + } + } + }; + + /** + * Creates a validator that checks if a string value is in an allowed set + */ + public static FieldValidator allowedValuesValidator(Set allowedValues, String errorMessage) { + return (fieldName, value) -> { + if (value instanceof String) { + String strValue = (String) value; + if (!allowedValues.contains(strValue)) { + throw new IllegalArgumentException( + errorMessage != null ? errorMessage : fieldName + " must be one of: " + String.join(", ", allowedValues) + ); + } + } + }; + } + + /** + * Creates a validator that checks if a string value is in an allowed list + */ + public static FieldValidator allowedValuesValidator(java.util.List allowedValues) { + return allowedValuesValidator(new HashSet<>(allowedValues), null); + } + + /** + * Combines multiple validators into one + */ + public static FieldValidator combineValidators(FieldValidator... validators) { + return (fieldName, value) -> { + for (FieldValidator validator : validators) { + validator.validate(fieldName, value); + } + }; + } } diff --git a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java index a80bf05b1a..77ea71eafc 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java @@ -579,7 +579,7 @@ public void fetchSharingInfo(String resourceIndex, String resourceId, ActionList .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, getResponse.getSourceAsString()) ) { parser.nextToken(); - ResourceSharing resourceSharing = ResourceSharing.fromXContent(parser, resourcePluginInfo.availableAccessLevels()); + ResourceSharing resourceSharing = ResourceSharing.fromXContent(parser); resourceSharing.setResourceId(getResponse.getId()); LOGGER.debug( @@ -995,7 +995,7 @@ public void fetchAccessibleResourceSharingRecords( ) ) { p.nextToken(); - ResourceSharing rs = ResourceSharing.fromXContent(p, resourcePluginInfo.availableAccessLevels()); + ResourceSharing rs = ResourceSharing.fromXContent(p); boolean canShare = canUserShare(user, /* isAdmin */ false, rs, resourceType); out.add(new SharingRecord(rs, canShare)); } catch (Exception ex) { @@ -1104,7 +1104,7 @@ private void processScrollResultsAndCollectSharingRecords( ) ) { parser.nextToken(); - ResourceSharing rs = ResourceSharing.fromXContent(parser, resourcePluginInfo.availableAccessLevels()); + ResourceSharing rs = ResourceSharing.fromXContent(parser); boolean canShare = canUserShare(user, isAdmin, rs, resourceType); resourceSharingRecords.add(new SharingRecord(rs, canShare)); } catch (Exception e) { diff --git a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java index 231db3697b..a7eda8c464 100644 --- a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java +++ b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java @@ -60,7 +60,6 @@ import org.opensearch.security.resources.sharing.Recipients; import org.opensearch.security.resources.sharing.ResourceSharing; import org.opensearch.security.resources.sharing.ShareWith; -import org.opensearch.security.resources.utils.InputValidation; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.spi.resources.ResourceProvider; import org.opensearch.threadpool.ThreadPool; @@ -155,36 +154,18 @@ private void handleMigrate(RestChannel channel, RestRequest request, Client clie private ValidationResult loadCurrentSharingInfo(RestRequest request, Client client) throws IOException { JsonNode body = Utils.toJsonNode(request.content().utf8ToString()); - // Required fields - String sourceIndex = InputValidation.getRequiredText(body, "source_index", InputValidation.MAX_INDEX_NAME_LENGTH); - String userNamePath = InputValidation.getRequiredText(body, "username_path", InputValidation.MAX_PATH_LENGTH); - String backendRolesPath = InputValidation.getRequiredText(body, "backend_roles_path", InputValidation.MAX_PATH_LENGTH); + // Extract fields - validation already done by RequestContentValidator framework + String sourceIndex = body.get("source_index").asText(); + String userNamePath = body.get("username_path").asText(); + String backendRolesPath = body.get("backend_roles_path").asText(); - // Optional - String defaultOwner = InputValidation.getOptionalText(body, "default_owner", InputValidation.MAX_PRINCIPAL_LENGTH); + // Optional field + JsonNode defaultOwnerNode = body.get("default_owner"); + String defaultOwner = (defaultOwnerNode != null && !defaultOwnerNode.isNull()) ? defaultOwnerNode.asText() : null; // Raw JSON for default_access_level JsonNode defaultAccessNode = body.get("default_access_level"); - // Validate JSON structure for default_access_level (object, non-empty, values non-empty) - try { - InputValidation.validateDefaultAccessLevelNode(defaultAccessNode); - } catch (IllegalArgumentException e) { - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(e.getMessage())); - } - - // Validate paths & owner early - InputValidation.validateJsonPath("username_path", userNamePath); - InputValidation.validateJsonPath("backend_roles_path", backendRolesPath); - InputValidation.validateDefaultOwner(defaultOwner); - - // Validate source index - try { - InputValidation.validateSourceIndex(sourceIndex, resourcePluginInfo.getResourceIndicesForProtectedTypes()); - } catch (Exception e) { - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(e.getMessage())); - } - // Convert after structural validation Map typeToDefaultAccessLevel = defaultAccessNode == null || defaultAccessNode.isNull() ? Collections.emptyMap() @@ -209,7 +190,7 @@ private ValidationResult loadCurrentSharingInfo(RestRequest Set accessLevels = resourcePluginInfo.flattenedForType(type).actionGroups(); try { - InputValidation.validateAccessLevel(defaultAccessLevelForType, accessLevels); + RequestContentValidator.validateAccessLevel(defaultAccessLevelForType, accessLevels); } catch (Exception e) { return ValidationResult.error( RestStatus.BAD_REQUEST, @@ -443,12 +424,79 @@ public Set mandatoryKeys() { @Override public Map allowedKeys() { + // Provide basic type information for backward compatibility return ImmutableMap.builder() - .put("source_index", RequestContentValidator.DataType.STRING) // name of the resource plugin index - .put("username_path", RequestContentValidator.DataType.STRING) // path to resource creator's name - .put("backend_roles_path", RequestContentValidator.DataType.STRING) // path to backend_roles - .put("default_owner", RequestContentValidator.DataType.STRING) // default owner name for resources without owner - .put("default_access_level", RequestContentValidator.DataType.OBJECT) // default access level by type + .put("source_index", RequestContentValidator.DataType.STRING) + .put("username_path", RequestContentValidator.DataType.STRING) + .put("backend_roles_path", RequestContentValidator.DataType.STRING) + .put("default_owner", RequestContentValidator.DataType.STRING) + .put("default_access_level", RequestContentValidator.DataType.OBJECT) + .build(); + } + + @Override + public Map allowedKeysWithConfig() { + // Validate source_index is in allowed set + RequestContentValidator.FieldValidator sourceIndexValidator = (fieldName, value) -> { + if (value instanceof String) { + String strValue = (String) value; + RequestContentValidator.requireNonEmpty(fieldName, strValue); + Set allowedIndices = resourcePluginInfo.getResourceIndicesForProtectedTypes(); + if (allowedIndices == null || allowedIndices.isEmpty()) { + throw new IllegalStateException("No protected resource indices configured"); + } + if (!allowedIndices.contains(strValue)) { + throw new IllegalArgumentException( + "Invalid resource index [" + strValue + "]. Allowed indices: " + allowedIndices + ); + } + } + }; + + return ImmutableMap.builder() + .put( + "source_index", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.STRING, + RequestContentValidator.MAX_INDEX_NAME_LENGTH, + sourceIndexValidator + ) + ) + .put( + "username_path", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.STRING, + RequestContentValidator.MAX_PATH_LENGTH, + RequestContentValidator.JSON_PATH_VALIDATOR + ) + ) + .put( + "backend_roles_path", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.STRING, + RequestContentValidator.MAX_PATH_LENGTH, + RequestContentValidator.JSON_PATH_VALIDATOR + ) + ) + .put( + "default_owner", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.STRING, + RequestContentValidator.MAX_PRINCIPAL_LENGTH, + RequestContentValidator.PRINCIPAL_VALIDATOR + ) + ) + .put( + "default_access_level", + RequestContentValidator.FieldConfiguration.of( + RequestContentValidator.DataType.OBJECT, + (fieldName, value) -> { + if (value instanceof JsonNode) { + RequestContentValidator.validateDefaultAccessLevelNode((JsonNode) value); + } + } + ) + ) .build(); } }); diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java index 77480730a5..cc6a5fe98c 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java @@ -22,9 +22,9 @@ import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.xcontent.XContentParser; import org.opensearch.rest.RestRequest; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator; import org.opensearch.security.resources.ResourcePluginInfo; import org.opensearch.security.resources.sharing.ShareWith; -import org.opensearch.security.resources.utils.InputValidation; /** * This class represents a request to share access to a resource. @@ -194,6 +194,12 @@ public void parseContent(XContentParser xContentParser, ResourcePluginInfo resou final List allowedTypes = resourcePluginInfo.currentProtectedTypes(); final Set validAccessLevels = resourcePluginInfo.availableAccessLevels(); + // Create access level validator using the pre-built helper + final RequestContentValidator.FieldValidator accessLevelValidator = RequestContentValidator.allowedValuesValidator( + validAccessLevels, + null + ); + try (XContentParser parser = xContentParser) { XContentParser.Token token; // START_OBJECT while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { @@ -203,14 +209,14 @@ public void parseContent(XContentParser xContentParser, ResourcePluginInfo resou switch (name) { case "resource_id": String resourceId = parser.text(); - InputValidation.validateResourceId(resourceId); + RequestContentValidator.validateResourceId(resourceId); this.resourceId(resourceId); break; case "resource_type": String resourceType = parser.text(); // Validate type syntax + membership in dynamic protected-types list - InputValidation.validateResourceType(resourceType, allowedTypes); + RequestContentValidator.validateResourceType(resourceType, allowedTypes); // Resolve to index, using pluginInfo String indexName = resourcePluginInfo.indexByType(resourceType); @@ -224,13 +230,13 @@ public void parseContent(XContentParser xContentParser, ResourcePluginInfo resou this.resourceIndex(resourcePluginInfo.indexByType(resourceType)); break; case "share_with": - this.shareWith(ShareWith.fromXContent(parser, validAccessLevels)); + this.shareWith(ShareWith.fromXContent(parser, accessLevelValidator)); break; case "add": - this.add(ShareWith.fromXContent(parser, validAccessLevels)); + this.add(ShareWith.fromXContent(parser, accessLevelValidator)); break; case "revoke": - this.revoke(ShareWith.fromXContent(parser, validAccessLevels)); + this.revoke(ShareWith.fromXContent(parser, accessLevelValidator)); break; default: parser.skipChildren(); diff --git a/src/main/java/org/opensearch/security/resources/sharing/Recipients.java b/src/main/java/org/opensearch/security/resources/sharing/Recipients.java index ff0c89a72a..3520b06cda 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/Recipients.java +++ b/src/main/java/org/opensearch/security/resources/sharing/Recipients.java @@ -22,7 +22,7 @@ import org.opensearch.core.xcontent.ToXContentFragment; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; -import org.opensearch.security.resources.utils.InputValidation; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator; /** * This class represents the entities with which a resource is shared for a particular action-group. @@ -97,7 +97,17 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder.endObject(); } - public static Recipients fromXContent(XContentParser parser) throws IOException { + /** + * Parse Recipients from XContent with validators + * @param parser the XContent parser + * @param arraySizeValidator optional validator for array size (can be null) + * @param elementValidator optional validator for each array element value (can be null) + */ + public static Recipients fromXContent( + XContentParser parser, + RequestContentValidator.FieldValidator arraySizeValidator, + RequestContentValidator.FieldValidator elementValidator + ) throws IOException { Map> recipients = new HashMap<>(); XContentParser.Token token; @@ -122,10 +132,18 @@ public static Recipients fromXContent(XContentParser parser) throws IOException while (parser.nextToken() != XContentParser.Token.END_ARRAY) { count++; - InputValidation.validateArrayEntryCount(fieldName, count); + + // Validate array size if validator provided + if (arraySizeValidator != null) { + arraySizeValidator.validate(fieldName, count); + } String value = parser.text(); - InputValidation.validatePrincipalValue(fieldName, value); + + // Validate element value if validator provided + if (elementValidator != null) { + elementValidator.validate(fieldName, value); + } values.add(value); } diff --git a/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java b/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java index a29c12e035..4e58d784f5 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java @@ -195,7 +195,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder.endObject(); } - public static ResourceSharing fromXContent(XContentParser parser, Set validAccessLevels) throws IOException { + public static ResourceSharing fromXContent(XContentParser parser) throws IOException { Builder b = ResourceSharing.builder(); String currentFieldName = null; @@ -220,7 +220,7 @@ public static ResourceSharing fromXContent(XContentParser parser, Set va b.createdBy(CreatedBy.fromXContent(parser)); break; case "share_with": - b.shareWith(ShareWith.fromXContent(parser, validAccessLevels)); + b.shareWith(ShareWith.fromXContent(parser)); break; default: parser.skipChildren(); diff --git a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java index 75285c102a..f2e9c3eac8 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java @@ -22,7 +22,7 @@ import org.opensearch.core.xcontent.ToXContentFragment; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; -import org.opensearch.security.resources.utils.InputValidation; +import org.opensearch.security.dlic.rest.validation.RequestContentValidator; /** * This class contains information about whom a resource is shared with and what is the action-group associated with it. @@ -97,7 +97,20 @@ public XContentBuilder toXContent(XContentBuilder b, Params params) throws IOExc return b.endObject(); } - public static ShareWith fromXContent(XContentParser parser, Set validAccessLevels) throws IOException { + /** + * Parse ShareWith from XContent without validation + */ + public static ShareWith fromXContent(XContentParser parser) throws IOException { + return fromXContent(parser, null); + } + + /** + * Parse ShareWith from XContent with custom access level validator + * @param parser the XContent parser + * @param accessLevelValidator optional validator for access level field names (can be null) + */ + public static ShareWith fromXContent(XContentParser parser, RequestContentValidator.FieldValidator accessLevelValidator) + throws IOException { Map sharingInfo = new HashMap<>(); if (parser.currentToken() != XContentParser.Token.START_OBJECT) { @@ -110,12 +123,18 @@ public static ShareWith fromXContent(XContentParser parser, Set validAcc if (token == XContentParser.Token.FIELD_NAME) { String accessLevel = parser.currentName(); - // We don't expect access-levels for a type to be null unless the type doesn't exist - InputValidation.validateAccessLevel(accessLevel, validAccessLevels); + // Validate access level if validator is provided + if (accessLevelValidator != null) { + accessLevelValidator.validate("access_level", accessLevel); + } parser.nextToken(); - Recipients recipients = Recipients.fromXContent(parser); + Recipients recipients = Recipients.fromXContent( + parser, + RequestContentValidator.ARRAY_SIZE_VALIDATOR, + RequestContentValidator.PRINCIPAL_VALIDATOR + ); sharingInfo.put(accessLevel, recipients); } } diff --git a/src/main/java/org/opensearch/security/resources/utils/InputValidation.java b/src/main/java/org/opensearch/security/resources/utils/InputValidation.java deleted file mode 100644 index 802cd84007..0000000000 --- a/src/main/java/org/opensearch/security/resources/utils/InputValidation.java +++ /dev/null @@ -1,186 +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. - */ - -package org.opensearch.security.resources.utils; - -import java.util.List; -import java.util.Set; -import java.util.regex.Pattern; - -import com.fasterxml.jackson.databind.JsonNode; - -import org.opensearch.core.common.Strings; - -/** - * Validates user inputs supplied to resource-sharing REST APIs - */ -public final class InputValidation { - - private InputValidation() {} - - public static final int MAX_RESOURCE_ID_LENGTH = 256; - public static final int MAX_RESOURCE_TYPE_LENGTH = 256; - public static final int MAX_ACCESS_LEVEL_LENGTH = 256; - public static final int MAX_PRINCIPAL_LENGTH = 256; - public static final int MAX_PATH_LENGTH = 256; - public static final int MAX_INDEX_NAME_LENGTH = 256; - public static final int MAX_ARRAY_SIZE = 100_000; - - // Alphanumeric + _ - : OR : * - "*" is only allowed as standalone - private static final Pattern SAFE_VALUE = Pattern.compile("^(\\*|[A-Za-z0-9_:-]+)$"); - - /* ---------------------- generic helpers ---------------------- */ - - public static void requireNonEmpty(String fieldName, String value) { - if (Strings.isNullOrEmpty(value)) { - throw new IllegalArgumentException(fieldName + " must not be null or empty"); - } - } - - public static void validateMaxLength(String fieldName, String value, int maxLength) { - if (value.length() > maxLength) { - throw new IllegalArgumentException(fieldName + " length [" + value.length() + "] exceeds max [" + maxLength + "]"); - } - } - - public static void validateSafeValue(String fieldName, String value, int maxLength) { - requireNonEmpty(fieldName, value); - validateMaxLength(fieldName, value, maxLength); - if (!SAFE_VALUE.matcher(value).matches()) { - throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); - } - } - - public static void validateArrayEntryCount(String fieldName, int count) { - if (count > MAX_ARRAY_SIZE) { - throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); - } - } - - public static void validateResourceId(String resourceId) { - validateSafeValue("resource_id", resourceId, MAX_RESOURCE_ID_LENGTH); - } - - public static void validateResourceType(String resourceType, List allowedTypes) { - validateSafeValue("resource_type", resourceType, MAX_RESOURCE_TYPE_LENGTH); - - if (allowedTypes == null || allowedTypes.isEmpty()) { - throw new IllegalStateException("No protected resource types configured"); - } - - if (!allowedTypes.contains(resourceType)) { - throw new IllegalArgumentException("Unsupported resource_type [" + resourceType + "], allowed types: " + allowedTypes); - } - } - - public static void validatePrincipalValue(String fieldName, String value) { - // users / roles / backend_roles entries - validateSafeValue(fieldName, value, MAX_PRINCIPAL_LENGTH); - } - - public static void validateAccessLevel(String accessLevel, Set validAccessLevels) { - requireNonEmpty("access_level", accessLevel); - - validateMaxLength("access_level", accessLevel, MAX_ACCESS_LEVEL_LENGTH); - - if (!SAFE_VALUE.matcher(accessLevel).matches()) { - throw new IllegalArgumentException("Invalid access_level [" + accessLevel + "]. Allowed characters: A-Z a-z 0-9 _ - :"); - } - - // Check against configured access-level set - if (validAccessLevels == null || validAccessLevels.isEmpty()) { - throw new IllegalStateException("No access levels configured."); - } - - if (!validAccessLevels.contains(accessLevel)) { - throw new IllegalArgumentException( - "Invalid access_level [" + accessLevel + "]. Allowed values: " + String.join(", ", validAccessLevels) - ); - } - } - - /* -------- JSON helpers for migrate API -------- */ - - public static String getRequiredText(JsonNode body, String fieldName, int maxLength) { - JsonNode node = body.get(fieldName); - if (node == null || node.isNull() || !node.isTextual()) { - throw new IllegalArgumentException("Field [" + fieldName + "] is required and must be a non-empty string"); - } - String value = node.asText(); - requireNonEmpty(fieldName, value); - validateMaxLength(fieldName, value, maxLength); - return value; - } - - public static String getOptionalText(JsonNode body, String fieldName, int maxLength) { - JsonNode node = body.get(fieldName); - if (node == null || node.isNull()) { - return null; - } - if (!node.isTextual()) { - throw new IllegalArgumentException("Field [" + fieldName + "] must be a string when provided"); - } - String value = node.asText(); - if (value.isEmpty()) { - return null; - } - validateMaxLength(fieldName, value, maxLength); - return value; - } - - /* --------- migrate-specific primitives --------- */ - - public static void validateJsonPath(String fieldName, String path) { - requireNonEmpty(fieldName, path); - validateMaxLength(fieldName, path, MAX_PATH_LENGTH); - // simple rule: no whitespace anywhere - if (!path.equals(path.trim()) || path.chars().anyMatch(Character::isWhitespace)) { - throw new IllegalArgumentException(fieldName + " must not contain whitespace"); - } - } - - public static void validateSourceIndex(String sourceIndex, Set allowedIndices) { - requireNonEmpty("source_index", sourceIndex); - validateMaxLength("source_index", sourceIndex, MAX_INDEX_NAME_LENGTH); - if (allowedIndices == null || allowedIndices.isEmpty()) { - throw new IllegalStateException("No protected resource indices configured"); - } - if (!allowedIndices.contains(sourceIndex)) { - throw new IllegalArgumentException("Invalid resource index [" + sourceIndex + "]. Allowed indices: " + allowedIndices); - } - } - - public static void validateDefaultOwner(String defaultOwner) { - if (defaultOwner == null) { - return; // optional - } - validatePrincipalValue("default_owner", defaultOwner); - } - - public static void validateDefaultAccessLevelNode(JsonNode node) { - if (node == null || node.isNull()) { - return; // field is optional - } - - if (!node.isObject()) { - throw new IllegalArgumentException("default_access_level must be an object"); - } - - if (!node.fieldNames().hasNext()) { - throw new IllegalArgumentException("default_access_level cannot be empty"); - } - - // Validate values are non-empty strings - node.fields().forEachRemaining(entry -> { - JsonNode val = entry.getValue(); - if (!val.isTextual() || val.asText().isEmpty()) { - throw new IllegalArgumentException("default_access_level for type [" + entry.getKey() + "] must be a non-empty string"); - } - }); - } -} diff --git a/src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java b/src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java index 72ad0ae76a..b6204a03ce 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java @@ -12,7 +12,10 @@ package org.opensearch.security.dlic.rest.validation; import java.io.IOException; +import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; @@ -300,7 +303,7 @@ public Map allowedKeys() { ObjectNode payload = DefaultObjectMapper.objectMapper.createObjectNode().putObject("a"); payload.putArray("a").add("arrray"); - payload.put("b", true).put("d", "some_string").put("e", "true").put("f", 1); + payload.put("b", true).put("d", "some_string").put("e", false).put("f", 1); payload.putObject("c"); when(httpRequest.content()).thenReturn(new BytesArray(payload.toString())); @@ -327,4 +330,385 @@ private void assertErrorMessage(final JsonNode jsonNode, final RequestContentVal assertThat(jsonNode.get("reason").asText(), is(expectedValidationError.message())); } + /* ======================================================================== + * Tests for Static Utility Methods (moved from InputValidationTests) + * ======================================================================== */ + + private static String repeat(char c, int count) { + char[] arr = new char[count]; + Arrays.fill(arr, c); + return new String(arr); + } + + private static void expectThrows(Class expected, Runnable runnable) { + try { + runnable.run(); + org.junit.Assert.fail("Expected exception of type " + expected.getName()); + } catch (Throwable t) { + assertTrue( + "Unexpected exception type. Expected " + expected.getName() + " but got " + t.getClass().getName(), + expected.isInstance(t) + ); + } + } + + /* ---------------------- requireNonEmpty ---------------------- */ + + @Test + public void testRequireNonEmptyAcceptsNonEmpty() { + RequestContentValidator.requireNonEmpty("field", "value"); + } + + @Test + public void testRequireNonEmptyRejectsNull() { + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.requireNonEmpty("field", null)); + } + + @Test + public void testRequireNonEmptyRejectsEmpty() { + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.requireNonEmpty("field", "")); + } + + /* ---------------------- validateMaxLength ---------------------- */ + + @Test + public void testValidateMaxLengthAtBoundary() { + String value = repeat('a', 5); + RequestContentValidator.validateMaxLength("field", value, 5); + } + + @Test + public void testValidateMaxLengthRejectsOverLimit() { + String value = repeat('a', 6); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateMaxLength("field", value, 5)); + } + + /* ---------------------- validateSafeValue ---------------------- */ + + @Test + public void testValidateSafeValueAcceptsAllowedCharacters() { + String value = "Abc123_-:xyz"; + RequestContentValidator.validateSafeValue("field", value, 50); + } + + @Test + public void testValidateSafeValueRejectsInvalidCharacters() { + String value = "bad value$"; // space + $ + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateSafeValue("field", value, 50)); + } + + @Test + public void testValidateSafeValueRejectsTooLong() { + String value = repeat('a', 11); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateSafeValue("field", value, 10)); + } + + /* ---------------------- validateArrayEntryCount ---------------------- */ + + @Test + public void testValidateArrayEntryCountAtMaxBoundary() { + RequestContentValidator.validateArrayEntryCount("field", RequestContentValidator.MAX_ARRAY_SIZE); + } + + @Test + public void testValidateArrayEntryCountRejectsAboveMax() { + int overMax = RequestContentValidator.MAX_ARRAY_SIZE + 1; + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateArrayEntryCount("field", overMax)); + } + + /* ---------------------- validateResourceId ---------------------- */ + + @Test + public void testValidateResourceIdAcceptsValidId() { + String id = "resource_123-ABC:xyz"; + RequestContentValidator.validateResourceId(id); + } + + @Test + public void testValidateResourceIdRejectsInvalidCharacters() { + String id = "invalid id!"; // space + ! + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateResourceId(id)); + } + + @Test + public void testValidateResourceIdRejectsTooLong() { + String id = repeat('a', RequestContentValidator.MAX_RESOURCE_ID_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateResourceId(id)); + } + + /* ---------------------- validateResourceType ---------------------- */ + + @Test + public void testValidateResourceTypeAcceptsValidTypeInAllowedList() { + List allowedTypes = Arrays.asList("anomaly-detector", "forecaster", "ml-model"); + RequestContentValidator.validateResourceType("anomaly-detector", allowedTypes); + } + + @Test + public void testValidateResourceTypeRejectsInvalidCharacters() { + List allowedTypes = List.of("anomaly-detector"); + String resourceType = "anomaly detector"; // contains space + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateResourceType(resourceType, allowedTypes)); + } + + @Test + public void testValidateResourceTypeRejectsWhenNoAllowedTypesConfiguredNull() { + expectThrows(IllegalStateException.class, () -> RequestContentValidator.validateResourceType("anomaly-detector", null)); + } + + @Test + public void testValidateResourceTypeRejectsWhenNoAllowedTypesConfiguredEmpty() { + expectThrows( + IllegalStateException.class, + () -> RequestContentValidator.validateResourceType("anomaly-detector", Collections.emptyList()) + ); + } + + @Test + public void testValidateResourceTypeRejectsWhenTypeNotInAllowedList() { + List allowedTypes = Arrays.asList("anomaly-detector", "forecaster"); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateResourceType("ml-model", allowedTypes)); + } + + /* ---------------------- validatePrincipalValue ---------------------- */ + + @Test + public void testValidatePrincipalValueAcceptsValidPrincipal() { + RequestContentValidator.validatePrincipalValue("users", "user_123-role:1"); + } + + @Test + public void testValidatePrincipalValueRejectsInvalidCharacters() { + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.validatePrincipalValue("users", "user name") // space + ); + } + + @Test + public void testValidatePrincipalValueRejectsTooLong() { + String principal = repeat('u', RequestContentValidator.MAX_PRINCIPAL_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validatePrincipalValue("users", principal)); + } + + /* ---------------------- validateAccessLevel ---------------------- */ + + @Test + public void testValidateAccessLevelAcceptsValidValueInSet() { + Set allowed = new HashSet<>(Arrays.asList("rd_read_only", "rd_write", "forecast:read", "forecast:write")); + + RequestContentValidator.validateAccessLevel("rd_read_only", allowed); + RequestContentValidator.validateAccessLevel("forecast:write", allowed); + } + + @Test + public void testValidateAccessLevelRejectsNullAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel(null, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsEmptyAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel("", allowed)); + } + + @Test + public void testValidateAccessLevelRejectsTooLongAccessLevel() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + String longAccess = repeat('a', RequestContentValidator.MAX_ACCESS_LEVEL_LENGTH + 1); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel(longAccess, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsInvalidCharacters() { + Set allowed = new HashSet<>(List.of("rd_read_only")); + String invalid = "rd read"; // space + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel(invalid, allowed)); + } + + @Test + public void testValidateAccessLevelRejectsWhenNoAccessLevelsConfiguredNull() { + expectThrows(IllegalStateException.class, () -> RequestContentValidator.validateAccessLevel("rd_read_only", null)); + } + + @Test + public void testValidateAccessLevelRejectsWhenNoAccessLevelsConfiguredEmpty() { + expectThrows( + IllegalStateException.class, + () -> RequestContentValidator.validateAccessLevel("rd_read_only", Collections.emptySet()) + ); + } + + @Test + public void testValidateAccessLevelRejectsWhenNotInAllowedSet() { + Set allowed = new HashSet<>(Arrays.asList("rd_read_only", "rd_write")); + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateAccessLevel("forecast:read", allowed)); + } + + /* ---------------------- getRequiredText ---------------------- */ + + @Test + public void testGetRequiredTextReturnsValueWhenPresent() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"source_index\":\"index-1\"}"); + + String result = RequestContentValidator.getRequiredText(body, "source_index", RequestContentValidator.MAX_INDEX_NAME_LENGTH); + + assertThat(result, is("index-1")); + } + + @Test + public void testGetRequiredTextThrowsWhenMissing() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"other\":\"value\"}"); + + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.getRequiredText(body, "source_index", RequestContentValidator.MAX_INDEX_NAME_LENGTH) + ); + } + + @Test + public void testGetRequiredTextThrowsWhenNonTextual() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"source_index\":123}"); + + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.getRequiredText(body, "source_index", RequestContentValidator.MAX_INDEX_NAME_LENGTH) + ); + } + + /* ---------------------- getOptionalText ---------------------- */ + + @Test + public void testGetOptionalTextReturnsNullWhenMissing() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"other\":\"value\"}"); + + String result = RequestContentValidator.getOptionalText(body, "default_owner", RequestContentValidator.MAX_PRINCIPAL_LENGTH); + + assertNull(result); + } + + @Test + public void testGetOptionalTextReturnsValueWhenPresent() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"default_owner\":\"owner_1\"}"); + + String result = RequestContentValidator.getOptionalText(body, "default_owner", RequestContentValidator.MAX_PRINCIPAL_LENGTH); + + assertThat(result, is("owner_1")); + } + + @Test + public void testGetOptionalTextThrowsWhenNonTextual() throws Exception { + JsonNode body = DefaultObjectMapper.readTree("{\"default_owner\":123}"); + + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.getOptionalText(body, "default_owner", RequestContentValidator.MAX_PRINCIPAL_LENGTH) + ); + } + + /* ---------------------- validateJsonPath ---------------------- */ + + @Test + public void testValidateJsonPathAcceptsValidPath() { + RequestContentValidator.validateJsonPath("username_path", "user.details.name"); + } + + @Test + public void testValidateJsonPathRejectsEmpty() { + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateJsonPath("username_path", "")); + } + + @Test + public void testValidateJsonPathRejectsWhitespace() { + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateJsonPath("username_path", " user . name ")); + } + + /* ---------------------- validateSourceIndex ---------------------- */ + + @Test + public void testValidateSourceIndexAcceptsWhenInAllowedSet() { + Set allowed = new HashSet<>(); + allowed.add("index-1"); + allowed.add("index-2"); + + RequestContentValidator.validateSourceIndex("index-1", allowed); + } + + @Test + public void testValidateSourceIndexRejectsWhenNotInAllowedSet() { + Set allowed = new HashSet<>(); + allowed.add("index-1"); + + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateSourceIndex("index-2", allowed)); + } + + @Test + public void testValidateSourceIndexRejectsWhenNoIndicesConfiguredNull() { + expectThrows(IllegalStateException.class, () -> RequestContentValidator.validateSourceIndex("index-1", null)); + } + + @Test + public void testValidateSourceIndexRejectsWhenNoIndicesConfiguredEmpty() { + expectThrows(IllegalStateException.class, () -> RequestContentValidator.validateSourceIndex("index-1", Collections.emptySet())); + } + + /* ---------------------- validateDefaultOwner ---------------------- */ + + @Test + public void testValidateDefaultOwnerAllowsNull() { + // optional field + RequestContentValidator.validateDefaultOwner(null); + } + + @Test + public void testValidateDefaultOwnerAcceptsValidPrincipal() { + RequestContentValidator.validateDefaultOwner("owner_123-role:1"); + } + + @Test + public void testValidateDefaultOwnerRejectsInvalidCharacters() { + expectThrows( + IllegalArgumentException.class, + () -> RequestContentValidator.validateDefaultOwner("owner name") // space + ); + } + + /* ---------------------- validateDefaultAccessLevelNode ---------------------- */ + + @Test + public void testValidateDefaultAccessLevelNodeAllowsNull() { + // field absent / null is allowed (optional) + RequestContentValidator.validateDefaultAccessLevelNode(null); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsNonObject() throws Exception { + JsonNode node = DefaultObjectMapper.readTree("\"string-not-object\""); + + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsEmptyObject() throws Exception { + JsonNode node = DefaultObjectMapper.readTree("{}"); + + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeRejectsEmptyValue() throws Exception { + JsonNode node = DefaultObjectMapper.readTree("{\"anomaly-detector\":\"\"}"); + + expectThrows(IllegalArgumentException.class, () -> RequestContentValidator.validateDefaultAccessLevelNode(node)); + } + + @Test + public void testValidateDefaultAccessLevelNodeAcceptsNonEmptyValues() throws Exception { + JsonNode node = DefaultObjectMapper.readTree("{ \"anomaly-detector\": \"rd_read_only\", \"forecaster\": \"rd_write\" }"); + + // should not throw + RequestContentValidator.validateDefaultAccessLevelNode(node); + } + } diff --git a/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java b/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java index d06a6484db..ab3650ab1a 100644 --- a/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java +++ b/src/test/java/org/opensearch/security/resources/sharing/ShareWithTests.java @@ -58,7 +58,10 @@ public void testFromXContentWhenCurrentTokenIsNotStartObject() throws IOExceptio parser.nextToken(); - ShareWith shareWith = ShareWith.fromXContent(parser, Set.of("read_only")); + ShareWith shareWith = ShareWith.fromXContent( + parser, + org.opensearch.security.dlic.rest.validation.RequestContentValidator.allowedValuesValidator(Set.of("read_only"), null) + ); assertThat(shareWith, notNullValue()); Recipients readOnly = shareWith.atAccessLevel("read_only"); @@ -77,7 +80,7 @@ public void testFromXContentWithEmptyInput() throws IOException { String emptyJson = "{}"; XContentParser parser = XContentType.JSON.xContent().createParser(NamedXContentRegistry.EMPTY, null, emptyJson); - ShareWith result = ShareWith.fromXContent(parser, Set.of()); + ShareWith result = ShareWith.fromXContent(parser, null); assertThat(result, notNullValue()); assertThat(result.isPrivate(), is(true)); @@ -106,7 +109,13 @@ public void testFromXContentWithStartObject() throws IOException { parser.nextToken(); - ShareWith shareWith = ShareWith.fromXContent(parser, Set.of("read-only", "default")); + ShareWith shareWith = ShareWith.fromXContent( + parser, + org.opensearch.security.dlic.rest.validation.RequestContentValidator.allowedValuesValidator( + Set.of("read-only", "default"), + null + ) + ); assertThat(shareWith, notNullValue()); @@ -132,7 +141,7 @@ public void testFromXContentWithUnexpectedEndOfInput() throws IOException { when(mockParser.currentToken()).thenReturn(XContentParser.Token.START_OBJECT); when(mockParser.nextToken()).thenReturn(XContentParser.Token.END_OBJECT, (XContentParser.Token) null); - ShareWith result = ShareWith.fromXContent(mockParser, Set.of()); + ShareWith result = ShareWith.fromXContent(mockParser, null); assertThat(result, notNullValue()); assertThat(result.isPrivate(), is(true)); @@ -202,7 +211,7 @@ public void test_fromXContent_emptyObject() throws IOException { parser = XContentType.JSON.xContent().createParser(null, null, builder.toString()); } - ShareWith shareWith = ShareWith.fromXContent(parser, Set.of()); + ShareWith shareWith = ShareWith.fromXContent(parser, null); assertThat(shareWith.isPrivate(), is(true)); assertThat(shareWith.isPublic(), is(false)); diff --git a/src/test/java/org/opensearch/security/resources/utils/InputValidationTests.java b/src/test/java/org/opensearch/security/resources/utils/InputValidationTests.java deleted file mode 100644 index 8a88d731ac..0000000000 --- a/src/test/java/org/opensearch/security/resources/utils/InputValidationTests.java +++ /dev/null @@ -1,406 +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. - */ - -package org.opensearch.security.resources.utils; - -import java.util.Arrays; -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Set; - -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; -import org.junit.Test; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; - -public class InputValidationTests { - - private static final ObjectMapper MAPPER = new ObjectMapper(); - - /* ---------------------- helpers ---------------------- */ - - private static String repeat(char c, int count) { - char[] arr = new char[count]; - Arrays.fill(arr, c); - return new String(arr); - } - - private static void expectThrows(Class expected, Runnable runnable) { - try { - runnable.run(); - fail("Expected exception of type " + expected.getName()); - } catch (Throwable t) { - assertTrue( - "Unexpected exception type. Expected " + expected.getName() + " but got " + t.getClass().getName(), - expected.isInstance(t) - ); - @SuppressWarnings("unchecked") - T cast = (T) t; - } - } - - private static JsonNode json(String s) throws Exception { - return MAPPER.readTree(s); - } - - /* ---------------------- requireNonEmpty ---------------------- */ - - @Test - public void testRequireNonEmptyAcceptsNonEmpty() { - InputValidation.requireNonEmpty("field", "value"); - } - - @Test - public void testRequireNonEmptyRejectsNull() { - expectThrows(IllegalArgumentException.class, () -> InputValidation.requireNonEmpty("field", null)); - } - - @Test - public void testRequireNonEmptyRejectsEmpty() { - expectThrows(IllegalArgumentException.class, () -> InputValidation.requireNonEmpty("field", "")); - } - - /* ---------------------- validateMaxLength ---------------------- */ - - @Test - public void testValidateMaxLengthAtBoundary() { - String value = repeat('a', 5); - InputValidation.validateMaxLength("field", value, 5); - } - - @Test - public void testValidateMaxLengthRejectsOverLimit() { - String value = repeat('a', 6); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateMaxLength("field", value, 5)); - } - - /* ---------------------- validateSafeValue ---------------------- */ - - @Test - public void testValidateSafeValueAcceptsAllowedCharacters() { - String value = "Abc123_-:xyz"; - InputValidation.validateSafeValue("field", value, 50); - } - - @Test - public void testValidateSafeValueRejectsInvalidCharacters() { - String value = "bad value$"; // space + $ - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateSafeValue("field", value, 50)); - } - - @Test - public void testValidateSafeValueRejectsTooLong() { - String value = repeat('a', 11); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateSafeValue("field", value, 10)); - } - - /* ---------------------- validateArrayEntryCount ---------------------- */ - - @Test - public void testValidateArrayEntryCountAtMaxBoundary() { - InputValidation.validateArrayEntryCount("field", InputValidation.MAX_ARRAY_SIZE); - } - - @Test - public void testValidateArrayEntryCountRejectsAboveMax() { - int overMax = InputValidation.MAX_ARRAY_SIZE + 1; - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateArrayEntryCount("field", overMax)); - } - - /* ---------------------- validateResourceId ---------------------- */ - - @Test - public void testValidateResourceIdAcceptsValidId() { - String id = "resource_123-ABC:xyz"; - InputValidation.validateResourceId(id); - } - - @Test - public void testValidateResourceIdRejectsInvalidCharacters() { - String id = "invalid id!"; // space + ! - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateResourceId(id)); - } - - @Test - public void testValidateResourceIdRejectsTooLong() { - String id = repeat('a', InputValidation.MAX_RESOURCE_ID_LENGTH + 1); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateResourceId(id)); - } - - /* ---------------------- validateResourceType ---------------------- */ - - @Test - public void testValidateResourceTypeAcceptsValidTypeInAllowedList() { - List allowedTypes = Arrays.asList("anomaly-detector", "forecaster", "ml-model"); - InputValidation.validateResourceType("anomaly-detector", allowedTypes); - } - - @Test - public void testValidateResourceTypeRejectsInvalidCharacters() { - List allowedTypes = List.of("anomaly-detector"); - String resourceType = "anomaly detector"; // contains space - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateResourceType(resourceType, allowedTypes)); - } - - @Test - public void testValidateResourceTypeRejectsWhenNoAllowedTypesConfiguredNull() { - expectThrows(IllegalStateException.class, () -> InputValidation.validateResourceType("anomaly-detector", null)); - } - - @Test - public void testValidateResourceTypeRejectsWhenNoAllowedTypesConfiguredEmpty() { - expectThrows(IllegalStateException.class, () -> InputValidation.validateResourceType("anomaly-detector", Collections.emptyList())); - } - - @Test - public void testValidateResourceTypeRejectsWhenTypeNotInAllowedList() { - List allowedTypes = Arrays.asList("anomaly-detector", "forecaster"); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateResourceType("ml-model", allowedTypes)); - } - - /* ---------------------- validatePrincipalValue ---------------------- */ - - @Test - public void testValidatePrincipalValueAcceptsValidPrincipal() { - InputValidation.validatePrincipalValue("users", "user_123-role:1"); - } - - @Test - public void testValidatePrincipalValueRejectsInvalidCharacters() { - expectThrows( - IllegalArgumentException.class, - () -> InputValidation.validatePrincipalValue("users", "user name") // space - ); - } - - @Test - public void testValidatePrincipalValueRejectsTooLong() { - String principal = repeat('u', InputValidation.MAX_PRINCIPAL_LENGTH + 1); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validatePrincipalValue("users", principal)); - } - - /* ---------------------- validateAccessLevel ---------------------- */ - - @Test - public void testValidateAccessLevelAcceptsValidValueInSet() { - Set allowed = new HashSet<>(Arrays.asList("rd_read_only", "rd_write", "forecast:read", "forecast:write")); - - InputValidation.validateAccessLevel("rd_read_only", allowed); - InputValidation.validateAccessLevel("forecast:write", allowed); - } - - @Test - public void testValidateAccessLevelRejectsNullAccessLevel() { - Set allowed = new HashSet<>(List.of("rd_read_only")); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel(null, allowed)); - } - - @Test - public void testValidateAccessLevelRejectsEmptyAccessLevel() { - Set allowed = new HashSet<>(List.of("rd_read_only")); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel("", allowed)); - } - - @Test - public void testValidateAccessLevelRejectsTooLongAccessLevel() { - Set allowed = new HashSet<>(List.of("rd_read_only")); - String longAccess = repeat('a', InputValidation.MAX_ACCESS_LEVEL_LENGTH + 1); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel(longAccess, allowed)); - } - - @Test - public void testValidateAccessLevelRejectsInvalidCharacters() { - Set allowed = new HashSet<>(List.of("rd_read_only")); - String invalid = "rd read"; // space - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel(invalid, allowed)); - } - - @Test - public void testValidateAccessLevelRejectsWhenNoAccessLevelsConfiguredNull() { - expectThrows(IllegalStateException.class, () -> InputValidation.validateAccessLevel("rd_read_only", null)); - } - - @Test - public void testValidateAccessLevelRejectsWhenNoAccessLevelsConfiguredEmpty() { - expectThrows(IllegalStateException.class, () -> InputValidation.validateAccessLevel("rd_read_only", Collections.emptySet())); - } - - @Test - public void testValidateAccessLevelRejectsWhenNotInAllowedSet() { - Set allowed = new HashSet<>(Arrays.asList("rd_read_only", "rd_write")); - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateAccessLevel("forecast:read", allowed)); - } - - @Test - public void testGetRequiredTextReturnsValueWhenPresent() throws Exception { - JsonNode body = json("{\"source_index\":\"index-1\"}"); - - String result = InputValidation.getRequiredText(body, "source_index", InputValidation.MAX_INDEX_NAME_LENGTH); - - assertEquals("index-1", result); - } - - @Test - public void testGetRequiredTextThrowsWhenMissing() throws Exception { - JsonNode body = json("{\"other\":\"value\"}"); - - expectThrows( - IllegalArgumentException.class, - () -> InputValidation.getRequiredText(body, "source_index", InputValidation.MAX_INDEX_NAME_LENGTH) - ); - } - - @Test - public void testGetRequiredTextThrowsWhenNonTextual() throws Exception { - JsonNode body = json("{\"source_index\":123}"); - - expectThrows( - IllegalArgumentException.class, - () -> InputValidation.getRequiredText(body, "source_index", InputValidation.MAX_INDEX_NAME_LENGTH) - ); - } - - /* ---------------------- getOptionalText ---------------------- */ - - @Test - public void testGetOptionalTextReturnsNullWhenMissing() throws Exception { - JsonNode body = json("{\"other\":\"value\"}"); - - String result = InputValidation.getOptionalText(body, "default_owner", InputValidation.MAX_PRINCIPAL_LENGTH); - - assertNull(result); - } - - @Test - public void testGetOptionalTextReturnsValueWhenPresent() throws Exception { - JsonNode body = json("{\"default_owner\":\"owner_1\"}"); - - String result = InputValidation.getOptionalText(body, "default_owner", InputValidation.MAX_PRINCIPAL_LENGTH); - - assertEquals("owner_1", result); - } - - @Test - public void testGetOptionalTextThrowsWhenNonTextual() throws Exception { - JsonNode body = json("{\"default_owner\":123}"); - - expectThrows( - IllegalArgumentException.class, - () -> InputValidation.getOptionalText(body, "default_owner", InputValidation.MAX_PRINCIPAL_LENGTH) - ); - } - - /* ---------------------- validateJsonPath ---------------------- */ - - @Test - public void testValidateJsonPathAcceptsValidPath() { - InputValidation.validateJsonPath("username_path", "user.details.name"); - } - - @Test - public void testValidateJsonPathRejectsEmpty() { - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateJsonPath("username_path", "")); - } - - @Test - public void testValidateJsonPathRejectsWhitespace() { - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateJsonPath("username_path", " user . name ")); - } - - /* ---------------------- validateSourceIndex ---------------------- */ - - @Test - public void testValidateSourceIndexAcceptsWhenInAllowedSet() { - Set allowed = new HashSet<>(); - allowed.add("index-1"); - allowed.add("index-2"); - - InputValidation.validateSourceIndex("index-1", allowed); - } - - @Test - public void testValidateSourceIndexRejectsWhenNotInAllowedSet() { - Set allowed = new HashSet<>(); - allowed.add("index-1"); - - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateSourceIndex("index-2", allowed)); - } - - @Test - public void testValidateSourceIndexRejectsWhenNoIndicesConfiguredNull() { - expectThrows(IllegalStateException.class, () -> InputValidation.validateSourceIndex("index-1", null)); - } - - @Test - public void testValidateSourceIndexRejectsWhenNoIndicesConfiguredEmpty() { - expectThrows(IllegalStateException.class, () -> InputValidation.validateSourceIndex("index-1", Collections.emptySet())); - } - - /* ---------------------- validateDefaultOwner ---------------------- */ - - @Test - public void testValidateDefaultOwnerAllowsNull() { - // optional field - InputValidation.validateDefaultOwner(null); - } - - @Test - public void testValidateDefaultOwnerAcceptsValidPrincipal() { - InputValidation.validateDefaultOwner("owner_123-role:1"); - } - - @Test - public void testValidateDefaultOwnerRejectsInvalidCharacters() { - expectThrows( - IllegalArgumentException.class, - () -> InputValidation.validateDefaultOwner("owner name") // space - ); - } - - /* ---------------------- validateDefaultAccessLevelNode ---------------------- */ - - @Test - public void testValidateDefaultAccessLevelNodeAllowsNull() { - // field absent / null is allowed (optional) - InputValidation.validateDefaultAccessLevelNode(null); - } - - @Test - public void testValidateDefaultAccessLevelNodeRejectsNonObject() throws Exception { - JsonNode node = json("\"string-not-object\""); - - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateDefaultAccessLevelNode(node)); - } - - @Test - public void testValidateDefaultAccessLevelNodeRejectsEmptyObject() throws Exception { - JsonNode node = json("{}"); - - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateDefaultAccessLevelNode(node)); - } - - @Test - public void testValidateDefaultAccessLevelNodeRejectsEmptyValue() throws Exception { - JsonNode node = json("{\"anomaly-detector\":\"\"}"); - - expectThrows(IllegalArgumentException.class, () -> InputValidation.validateDefaultAccessLevelNode(node)); - } - - @Test - public void testValidateDefaultAccessLevelNodeAcceptsNonEmptyValues() throws Exception { - JsonNode node = json("{ \"anomaly-detector\": \"rd_read_only\", \"forecaster\": \"rd_write\" }"); - - // should not throw - InputValidation.validateDefaultAccessLevelNode(node); - } -} From d18aed55eadec5e56a259275b25e5ee684027316 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 4 Dec 2025 17:05:10 -0800 Subject: [PATCH 8/9] Fix broken CI Signed-off-by: Darshit Chanpura --- .../validation/RequestContentValidator.java | 30 +++++++------------ .../MigrateResourceSharingInfoApiAction.java | 13 ++++---- .../test/TenancyMultitenancyEnabledTests.java | 2 +- .../TenancyPrivateTenantEnabledTests.java | 2 +- 4 files changed, 19 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java index ed9ad829ec..0c58355d6a 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java @@ -86,7 +86,7 @@ public static enum DataType { } /** - * Validator interface for field-level validation, similar to OpenSearch Setting.Validator + * Validator interface for field-level validation */ @FunctionalInterface public interface FieldValidator { @@ -143,8 +143,7 @@ public FieldValidator getValidator() { public void validate(String fieldName, Object value) { // Validate max length for strings - if (maxLength != null && value instanceof String) { - String strValue = (String) value; + if (maxLength != null && value instanceof String strValue) { if (strValue.length() > maxLength) { throw new IllegalArgumentException(fieldName + " length [" + strValue.length() + "] exceeds max [" + maxLength + "]"); } @@ -285,13 +284,13 @@ private ValidationResult validateDataType(final JsonNode jsonContent) JsonToken token; while ((token = parser.nextToken()) != null) { if (token.equals(JsonToken.FIELD_NAME)) { - String currentName = parser.getCurrentName(); + String currentName = parser.currentName(); // Get data type from either FieldConfiguration or simple DataType map final DataType dataType; final FieldConfiguration fieldConfig; - if (useEnhancedValidation && fieldConfigs != null) { + if (useEnhancedValidation) { fieldConfig = fieldConfigs.get(currentName); dataType = (fieldConfig != null) ? fieldConfig.getDataType() : null; } else { @@ -703,8 +702,7 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { * and optionally allows standalone "*" */ public static final FieldValidator SAFE_VALUE_VALIDATOR = (fieldName, value) -> { - if (value instanceof String) { - String strValue = (String) value; + if (value instanceof String strValue) { requireNonEmpty(fieldName, strValue); if (!SAFE_VALUE.matcher(strValue).matches()) { throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); @@ -716,8 +714,7 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { * Validator for resource IDs */ public static final FieldValidator RESOURCE_ID_VALIDATOR = (fieldName, value) -> { - if (value instanceof String) { - String strValue = (String) value; + if (value instanceof String strValue) { requireNonEmpty(fieldName, strValue); validateMaxLength(fieldName, strValue, MAX_RESOURCE_ID_LENGTH); if (!SAFE_VALUE.matcher(strValue).matches()) { @@ -730,8 +727,7 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { * Validator for principal values (users, roles, backend_roles) */ public static final FieldValidator PRINCIPAL_VALIDATOR = (fieldName, value) -> { - if (value instanceof String) { - String strValue = (String) value; + if (value instanceof String strValue) { requireNonEmpty(fieldName, strValue); validateMaxLength(fieldName, strValue, MAX_PRINCIPAL_LENGTH); if (!SAFE_VALUE.matcher(strValue).matches()) { @@ -744,8 +740,7 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { * Validator for JSON paths (no whitespace allowed) */ public static final FieldValidator JSON_PATH_VALIDATOR = (fieldName, value) -> { - if (value instanceof String) { - String strValue = (String) value; + if (value instanceof String strValue) { requireNonEmpty(fieldName, strValue); validateMaxLength(fieldName, strValue, MAX_PATH_LENGTH); if (!strValue.equals(strValue.trim()) || strValue.chars().anyMatch(Character::isWhitespace)) { @@ -758,8 +753,7 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { * Validator for array entry counts (works with JsonNode arrays) */ public static final FieldValidator ARRAY_SIZE_VALIDATOR = (fieldName, value) -> { - if (value instanceof JsonNode) { - JsonNode node = (JsonNode) value; + if (value instanceof JsonNode node) { if (node.isArray() && node.size() > MAX_ARRAY_SIZE) { throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE); } @@ -775,8 +769,7 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { * Validator that ensures array elements are non-null and non-blank */ public static final FieldValidator NON_EMPTY_ARRAY_ELEMENTS_VALIDATOR = (fieldName, value) -> { - if (value instanceof JsonNode) { - JsonNode node = (JsonNode) value; + if (value instanceof JsonNode node) { if (node.isArray()) { for (JsonNode element : node) { if (element.isNull() || (element.isTextual() && Strings.isNullOrEmpty(element.asText().trim()))) { @@ -792,8 +785,7 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { */ public static FieldValidator allowedValuesValidator(Set allowedValues, String errorMessage) { return (fieldName, value) -> { - if (value instanceof String) { - String strValue = (String) value; + if (value instanceof String strValue) { if (!allowedValues.contains(strValue)) { throw new IllegalArgumentException( errorMessage != null ? errorMessage : fieldName + " must be one of: " + String.join(", ", allowedValues) diff --git a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java index a7eda8c464..88794a1a3d 100644 --- a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java +++ b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java @@ -181,7 +181,9 @@ private ValidationResult loadCurrentSharingInfo(RestRequest // Validate resource type exists ResourceProvider provider = resourcePluginInfo.getResourceProvider(type); if (provider == null) { - return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Invalid resource type " + type + ".")); + // We do not expect this to be null, as validation check will already have been performed in request content validator + String message = String.format("resource_type be one of: %s", resourcePluginInfo.currentProtectedTypes()); + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(message)); } typePath = provider.typeField(); @@ -438,17 +440,14 @@ public Map allowedKeys() { public Map allowedKeysWithConfig() { // Validate source_index is in allowed set RequestContentValidator.FieldValidator sourceIndexValidator = (fieldName, value) -> { - if (value instanceof String) { - String strValue = (String) value; + if (value instanceof String strValue) { RequestContentValidator.requireNonEmpty(fieldName, strValue); Set allowedIndices = resourcePluginInfo.getResourceIndicesForProtectedTypes(); if (allowedIndices == null || allowedIndices.isEmpty()) { - throw new IllegalStateException("No protected resource indices configured"); + throw new IllegalStateException("No protected resources configured"); } if (!allowedIndices.contains(strValue)) { - throw new IllegalArgumentException( - "Invalid resource index [" + strValue + "]. Allowed indices: " + allowedIndices - ); + throw new IllegalArgumentException("source_index must be one of: " + allowedIndices); } } }; diff --git a/src/test/java/org/opensearch/security/multitenancy/test/TenancyMultitenancyEnabledTests.java b/src/test/java/org/opensearch/security/multitenancy/test/TenancyMultitenancyEnabledTests.java index 32b9bb2156..2075636eec 100644 --- a/src/test/java/org/opensearch/security/multitenancy/test/TenancyMultitenancyEnabledTests.java +++ b/src/test/java/org/opensearch/security/multitenancy/test/TenancyMultitenancyEnabledTests.java @@ -92,7 +92,7 @@ public void testMultitenancyDisabled_endToEndTest() throws Exception { final HttpResponse updateMutlitenancyToDisabled = nonSslRestHelper().executePutRequest( "/_plugins/_security/api/tenancy/config", - "{\"multitenancy_enabled\": \"false\"}", + "{\"multitenancy_enabled\": false}", AS_REST_API_USER ); assertThat(updateMutlitenancyToDisabled.getStatusCode(), equalTo(HttpStatus.SC_OK)); diff --git a/src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java b/src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java index 4f2d2c3505..e3a59697e3 100644 --- a/src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java +++ b/src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java @@ -93,7 +93,7 @@ public void testPrivateTenantDisabled_Update_EndToEnd() throws Exception { final HttpResponse disablePrivateTenantResponse = nonSslRestHelper().executePutRequest( "/_plugins/_security/api/tenancy/config", - "{\"private_tenant_enabled\": \"false\"}", + "{\"private_tenant_enabled\": false}", AS_REST_API_USER ); assertThat(disablePrivateTenantResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); From ca3e104ce263267949a4091afda26f8adc2a907d Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 4 Dec 2025 18:32:47 -0800 Subject: [PATCH 9/9] Corrects the validators for migrate api action Signed-off-by: Darshit Chanpura --- .../securityapis/MigrateApiTests.java | 6 +- .../validation/RequestContentValidator.java | 74 ++++--------------- .../MigrateResourceSharingInfoApiAction.java | 13 +++- 3 files changed, 28 insertions(+), 65 deletions(-) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java index fea697c2b6..7bf0c0011a 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/securityapis/MigrateApiTests.java @@ -433,7 +433,7 @@ public void testMigrateAPI_inputValidation_invalidValues() { TestRestClient.HttpResponse response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidUserPathPayload); - assertThat(response, RestMatchers.isBadRequest("/error/reason", "username_path must not contain whitespace")); + assertThat(response, RestMatchers.isBadRequest("/message", "username_path must not contain whitespace")); // ------------------------------ // 2) Invalid backend_roles_path (whitespace) @@ -452,7 +452,7 @@ public void testMigrateAPI_inputValidation_invalidValues() { response = client.postJson(RESOURCE_SHARING_MIGRATION_ENDPOINT, invalidBackendPathPayload); - assertThat(response, RestMatchers.isBadRequest("/error/reason", "backend_roles_path must not contain whitespace")); + assertThat(response, RestMatchers.isBadRequest("/message", "backend_roles_path must not contain whitespace")); // ------------------------------ // 3) Invalid default_owner (bad characters) @@ -473,7 +473,7 @@ public void testMigrateAPI_inputValidation_invalidValues() { assertThat( response, - RestMatchers.isBadRequest("/error/reason", "default_owner contains invalid characters; allowed: A-Z a-z 0-9 _ - :") + RestMatchers.isBadRequest("/message", "default_owner contains invalid characters; allowed: A-Z a-z 0-9 _ - :") ); // ------------------------------ diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java index 0c58355d6a..8e1120334e 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java @@ -32,6 +32,7 @@ import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.rest.RestRequest; import org.opensearch.security.DefaultObjectMapper; +import org.opensearch.security.resources.sharing.Recipient; import com.flipkart.zjsonpatch.JsonDiff; @@ -561,7 +562,12 @@ public static void validateSafeValue(String fieldName, String value, int maxLeng requireNonEmpty(fieldName, value); validateMaxLength(fieldName, value, maxLength); if (!SAFE_VALUE.matcher(value).matches()) { - throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); + throw new IllegalArgumentException( + fieldName + + " contains invalid characters; allowed: " + + (fieldName.equals(Recipient.USERS.getName()) ? "* OR " : "") + + "A-Z a-z 0-9 _ - :" + ); } } @@ -697,32 +703,6 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { * Pre-built Field Validators for Common Use Cases * ======================================================================== */ - /** - * Validator that ensures a string contains only safe characters (alphanumeric + _ - :) - * and optionally allows standalone "*" - */ - public static final FieldValidator SAFE_VALUE_VALIDATOR = (fieldName, value) -> { - if (value instanceof String strValue) { - requireNonEmpty(fieldName, strValue); - if (!SAFE_VALUE.matcher(strValue).matches()) { - throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); - } - } - }; - - /** - * Validator for resource IDs - */ - public static final FieldValidator RESOURCE_ID_VALIDATOR = (fieldName, value) -> { - if (value instanceof String strValue) { - requireNonEmpty(fieldName, strValue); - validateMaxLength(fieldName, strValue, MAX_RESOURCE_ID_LENGTH); - if (!SAFE_VALUE.matcher(strValue).matches()) { - throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); - } - } - }; - /** * Validator for principal values (users, roles, backend_roles) */ @@ -731,7 +711,12 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { requireNonEmpty(fieldName, strValue); validateMaxLength(fieldName, strValue, MAX_PRINCIPAL_LENGTH); if (!SAFE_VALUE.matcher(strValue).matches()) { - throw new IllegalArgumentException(fieldName + " contains invalid characters; allowed: A-Z a-z 0-9 _ - :"); + throw new IllegalArgumentException( + fieldName + + " contains invalid characters; allowed: " + + (fieldName.equals(Recipient.USERS.getName()) ? "* OR " : "") + + "A-Z a-z 0-9 _ - :" + ); } } }; @@ -765,21 +750,6 @@ public static void validateDefaultAccessLevelNode(JsonNode node) { } }; - /** - * Validator that ensures array elements are non-null and non-blank - */ - public static final FieldValidator NON_EMPTY_ARRAY_ELEMENTS_VALIDATOR = (fieldName, value) -> { - if (value instanceof JsonNode node) { - if (node.isArray()) { - for (JsonNode element : node) { - if (element.isNull() || (element.isTextual() && Strings.isNullOrEmpty(element.asText().trim()))) { - throw new IllegalArgumentException(fieldName + " contains null or blank array elements"); - } - } - } - } - }; - /** * Creates a validator that checks if a string value is in an allowed set */ @@ -794,22 +764,4 @@ public static FieldValidator allowedValuesValidator(Set allowedValues, S } }; } - - /** - * Creates a validator that checks if a string value is in an allowed list - */ - public static FieldValidator allowedValuesValidator(java.util.List allowedValues) { - return allowedValuesValidator(new HashSet<>(allowedValues), null); - } - - /** - * Combines multiple validators into one - */ - public static FieldValidator combineValidators(FieldValidator... validators) { - return (fieldName, value) -> { - for (FieldValidator validator : validators) { - validator.validate(fieldName, value); - } - }; - } } diff --git a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java index 88794a1a3d..3386c06b9f 100644 --- a/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java +++ b/src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java @@ -154,7 +154,7 @@ private void handleMigrate(RestChannel channel, RestRequest request, Client clie private ValidationResult loadCurrentSharingInfo(RestRequest request, Client client) throws IOException { JsonNode body = Utils.toJsonNode(request.content().utf8ToString()); - // Extract fields - validation already done by RequestContentValidator framework + // Extract fields - basic validation done by RequestContentValidator framework String sourceIndex = body.get("source_index").asText(); String userNamePath = body.get("username_path").asText(); String backendRolesPath = body.get("backend_roles_path").asText(); @@ -166,6 +166,17 @@ private ValidationResult loadCurrentSharingInfo(RestRequest // Raw JSON for default_access_level JsonNode defaultAccessNode = body.get("default_access_level"); + // Business logic validation (after framework validation) + try { + RequestContentValidator.validateJsonPath("username_path", userNamePath); + RequestContentValidator.validateJsonPath("backend_roles_path", backendRolesPath); + RequestContentValidator.validateDefaultOwner(defaultOwner); + RequestContentValidator.validateSourceIndex(sourceIndex, resourcePluginInfo.getResourceIndicesForProtectedTypes()); + RequestContentValidator.validateDefaultAccessLevelNode(defaultAccessNode); + } catch (IllegalArgumentException | IllegalStateException e) { + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage(e.getMessage())); + } + // Convert after structural validation Map typeToDefaultAccessLevel = defaultAccessNode == null || defaultAccessNode.isNull() ? Collections.emptyMap()