Update RequestContentValidator to only validate fields from request payload and not pre-existing values stored in security index

CHANGELOG.md

@@ -3,5 +3,35 @@
 As of the 3.6 release [the CHANGELOG is no longer used](https://github.com/opensearch-project/OpenSearch/issues/21071) to generate release notes.
 [Use this PR search](https://github.com/opensearch-project/security/pulls?q=sort%3Amerged-desc+is%3Apr+-label%3Askip-changelog+is%3Amerged+base%3Amain+) to browse unreleased changes.
 
+## [Unreleased 3.x]
+### Added
+- Add `plugins.security.audit.config.log4j.maximum_index_characters_per_message` setting to allow splitting of Log4j audit log messages to keep the `audit_trace_indices` and `audit_trace_resolved_indices` fields below the maximum characters specified. ([5977](https://github.com/opensearch-project/security/pull/5977))
+
+### Changed
+
+### Features
+
+### Enhancements
+- Make `plugins.security.dfm_empty_overrides_all` dynamically toggleable ([#6016](https://github.com/opensearch-project/security/pull/6016)
+- Cache FLS status information when processing index query cache on a node ([#6044](https://github.com/opensearch-project/security/pull/6044))
+- Only update internal compiled privileges configuration when the base config objects have actually changed ([#6037](https://github.com/opensearch-project/security/pull/6037))
+
+### Bug Fixes
+- Update RequestContentValidator to only validate fields from request payload and not pre-existing values stored in security index ([#6061](https://github.com/opensearch-project/security/pull/6061))
+
+### Refactoring
+
+### Maintenance
+- Bump `gradle-wrapper` from 9.4.0 to 9.4.1 ([#6049](https://github.com/opensearch-project/security/pull/6049))
+- Bump `1password/load-secrets-action` from 3 to 4 ([#6047](https://github.com/opensearch-project/security/pull/6047))
+- Bump `io.projectreactor:reactor-core` from 3.8.2 to 3.8.4 ([#6046](https://github.com/opensearch-project/security/pull/6046))
+- Bump `commons-logging:commons-logging` from 1.3.5 to 1.3.6 ([#6050](https://github.com/opensearch-project/security/pull/6050))
+- Bump `org.mockito:mockito-core` from 5.21.0 to 5.23.0 ([#6048](https://github.com/opensearch-project/security/pull/6048))
+
+### Removed
+
+### Documentation
+
+[Unreleased 3.x]: https://github.com/opensearch-project/security/compare/3.6...main
 Release notes are now auto-generated from PR metadata at release time using an LLM-based pipeline in [opensearch-build](https://github.com/opensearch-project/opensearch-build).
 See the [release notes script](https://github.com/opensearch-project/opensearch-build/blob/main/src/release_notes_workflow/release_notes.py) and [LLM prompt](https://github.com/opensearch-project/opensearch-build/blob/main/src/release_notes_workflow/release_notes_prompt.txt) for details.

src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java

@@ -66,12 +66,15 @@ public class InternalUsersRestApiIntegrationTest extends AbstractConfigEntityApi
 
     private final static String SOME_ROLE = "some-role";
 
+    private final static String LEGACY_EMPTY_BACKEND_ROLES_USER = "legacy_empty_backend_roles_user";
+
     @ClassRule
     public static LocalCluster localCluster = clusterBuilder().users(
         new TestSecurityConfig.User(SERVICE_ACCOUNT_USER).attr("service", "true").attr("enabled", "true"),
         new TestSecurityConfig.User(REST_API_ADMIN_INTERNAL_USERS_ONLY).referencedRoles(
             new TestSecurityConfig.Role(REST_API_ADMIN_INTERNAL_USERS_ONLY)
-        ).roles(new TestSecurityConfig.Role("rest_admin_role").clusterPermissions(restAdminPermission(Endpoint.INTERNALUSERS)))
+        ).roles(new TestSecurityConfig.Role("rest_admin_role").clusterPermissions(restAdminPermission(Endpoint.INTERNALUSERS))),
+        new TestSecurityConfig.User(LEGACY_EMPTY_BACKEND_ROLES_USER).backendRoles("", "admin")
     )
         .roles(
             new TestSecurityConfig.Role(HIDDEN_ROLE).hidden(true),
@@ -870,6 +873,31 @@ public void serviceUsers() throws Exception {
         }
     }
 
+    @Test
+    public void bulkPatchChangingHashDoesNotRevalidateUnchangedBackendRoles() throws Exception {
+        try (TestRestClient client = localCluster.getRestClient(ADMIN_USER)) {
+            // Bulk PATCH only the hash on a user with legacy empty-string backend_roles —
+            // should succeed without revalidating the unchanged backend_roles
+            assertThat(
+                client.patch(
+                    apiPath(),
+                    patch(
+                        replaceOp(LEGACY_EMPTY_BACKEND_ROLES_USER + "/hash", "$2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m")
+                    )
+                ),
+                isOk()
+            );
+            // Bulk PATCH that changes backend_roles with blank values — should still be rejected
+            assertThat(
+                client.patch(
+                    apiPath(),
+                    patch(replaceOp(LEGACY_EMPTY_BACKEND_ROLES_USER + "/backend_roles", configJsonArray("", "new_role")))
+                ),
+                isBadRequest()
+            );
+        }
+    }
+
     private String randomAsciiAlphanumOfLength(int length) {
         final var characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
         final var random = new Random();

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java

@@ -262,7 +262,11 @@ protected ValidationResult<SecurityConfiguration> patchEntities(
                 }
                 // create or update case of the entity. we need to verify new JSON configuration for them
                 if ((beforePatchEntity == null) || !Objects.equals(beforePatchEntity, patchedEntity)) {
-                    final var requestCheck = endpointValidator.createRequestContentValidator(entityName).validate(request, patchedEntity);
+                    final var validator = endpointValidator.createRequestContentValidator(entityName);
+                    if (beforePatchEntity != null) {
+                        validator.withOriginalContent(beforePatchEntity);
+                    }
+                    final var requestCheck = validator.validate(request, patchedEntity);
                     if (!requestCheck.isValid()) {
                         return ValidationResult.error(requestCheck.status(), requestCheck.errorMessage());
                     }

src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java

@@ -209,6 +209,17 @@ protected RequestContentValidator(final ValidationContext validationContext) {
         this.validationContext = validationContext;
     }
 
+    private JsonNode originalContent;
+
+    /**
+     * Sets the original content for patch-aware validation.
+     * When set, the null/blank array element check will only validate fields that changed.
+     */
+    public RequestContentValidator withOriginalContent(final JsonNode originalContent) {
+        this.originalContent = originalContent;
+        return this;
+    }
+
     public ValidationResult<JsonNode> validate(final RestRequest request) throws IOException {
         return parseRequestContent(request).map(this::validateContentSize).map(jsonContent -> validate(request, jsonContent));
     }
@@ -228,7 +239,7 @@ public ValidationResult<JsonNode> validate(final RestRequest request, final Json
         }
         return validateContentSize(patchedContent).map(this::validateJsonKeys)
             .map(this::validateDataType)
-            .map(this::nullValuesInArrayValidator)
+            .map(content -> nullValuesInArrayValidator(content, patch))
             .map(ignored -> validatePassword(request, patchedContent));
     }
 
@@ -381,6 +392,10 @@ private ValidationResult<JsonNode> validateDataType(final JsonNode jsonContent)
     }
 
     private ValidationResult<JsonNode> nullValuesInArrayValidator(final JsonNode jsonContent) {
+        if (originalContent != null) {
+            final JsonNode patch = JsonDiff.asJson(originalContent, jsonContent);
+            return nullValuesInArrayValidator(jsonContent, patch);
+        }
         // Use allowedKeysWithConfig if provided, otherwise fall back to allowedKeys
         final Map<String, FieldConfiguration> fieldConfigs = validationContext.allowedKeysWithConfig();
         final Set<String> allowedKeys = (fieldConfigs != null) ? fieldConfigs.keySet() : validationContext.allowedKeys().keySet();
@@ -397,6 +412,40 @@ private ValidationResult<JsonNode> nullValuesInArrayValidator(final JsonNode jso
         return ValidationResult.success(jsonContent);
     }
 
+    /**
+     * Validates only the fields that were modified by a PATCH operation.
+     * This allows existing documents with legacy empty-string values in unchanged fields
+     * to be updated without triggering validation errors on those untouched fields.
+     */
+    private ValidationResult<JsonNode> nullValuesInArrayValidator(final JsonNode jsonContent, final JsonNode patch) {
+        final Set<String> changedFields = new HashSet<>();
+        for (final JsonNode op : patch) {
+            final String path = op.get("path").asText();
+            // path is like "/fieldName" or "/entityName/fieldName" — extract the top-level field
+            final String[] parts = path.split("/");
+            if (parts.length >= 2) {
+                changedFields.add(parts[1]);
+            }
+        }
+        for (final Map.Entry<String, DataType> allowedKey : validationContext.allowedKeys().entrySet()) {
+            if (!changedFields.contains(allowedKey.getKey())) {
+                continue;
+            }
+            final DataType dataType = allowedKey.getValue();
+            if (dataType != DataType.ARRAY) {
+                continue;
+            }
+            JsonNode value = jsonContent.get(allowedKey.getKey());
+            if (value != null) {
+                if (hasNullOrBlankArrayElement(value)) {
+                    this.validationError = ValidationError.NULL_ARRAY_ELEMENT;
+                    return ValidationResult.error(RestStatus.BAD_REQUEST, this);
+                }
+            }
+        }
+        return ValidationResult.success(jsonContent);
+    }
+
     private boolean hasNullOrBlankArrayElement(final JsonNode node) {
         for (final JsonNode element : node) {
             if (element.isNull() || (element.isTextual() && Strings.isNullOrEmpty(element.asText().trim()))) {

src/test/java/org/opensearch/security/dlic/rest/validation/RequestContentValidatorTest.java

@@ -523,6 +523,68 @@ public Map<String, RequestContentValidator.DataType> allowedKeys() {
         assertTrue(validationResult.isValid());
     }
 
+    @Test
+    public void testPatchSkipsValidationOnUnchangedFieldsWithBlankArrayElements() throws Exception {
+        final RequestContentValidator validator = RequestContentValidator.of(new RequestContentValidator.ValidationContext() {
+            @Override
+            public Object[] params() {
+                return new Object[0];
+            }
+
+            @Override
+            public Settings settings() {
+                return Settings.EMPTY;
+            }
+
+            @Override
+            public Map<String, RequestContentValidator.DataType> allowedKeys() {
+                return Map.of("hash", RequestContentValidator.DataType.STRING, "backend_roles", RequestContentValidator.DataType.ARRAY);
+            }
+        });
+        // Original has a legacy empty-string in backend_roles
+        final ObjectNode original = DefaultObjectMapper.objectMapper.createObjectNode();
+        original.put("hash", "oldhash");
+        original.putArray("backend_roles").add("").add("admin");
+
+        // Patch only changes hash, backend_roles stays the same
+        final ObjectNode patched = original.deepCopy();
+        patched.put("hash", "newhash");
+
+        final ValidationResult<JsonNode> validationResult = validator.validate(request, patched, original);
+        assertTrue("PATCH on unrelated field should pass even with legacy blank array elements", validationResult.isValid());
+    }
+
+    @Test
+    public void testPatchRejectsBlankArrayElementsInChangedField() throws Exception {
+        final RequestContentValidator validator = RequestContentValidator.of(new RequestContentValidator.ValidationContext() {
+            @Override
+            public Object[] params() {
+                return new Object[0];
+            }
+
+            @Override
+            public Settings settings() {
+                return Settings.EMPTY;
+            }
+
+            @Override
+            public Map<String, RequestContentValidator.DataType> allowedKeys() {
+                return Map.of("hash", RequestContentValidator.DataType.STRING, "backend_roles", RequestContentValidator.DataType.ARRAY);
+            }
+        });
+        final ObjectNode original = DefaultObjectMapper.objectMapper.createObjectNode();
+        original.put("hash", "oldhash");
+        original.putArray("backend_roles").add("admin");
+
+        // Patch introduces a blank element in backend_roles
+        final ObjectNode patched = original.deepCopy();
+        patched.putArray("backend_roles").add("").add("admin");
+
+        final ValidationResult<JsonNode> validationResult = validator.validate(request, patched, original);
+        assertFalse("PATCH that introduces blank array elements should be rejected", validationResult.isValid());
+        assertErrorMessage(validationResult.errorMessage(), RequestContentValidator.ValidationError.NULL_ARRAY_ELEMENT);
+    }
+
     @Test
     public void testNoopValidator() throws Exception {
         final ValidationResult<JsonNode> validationResult = RequestContentValidator.NOOP_VALIDATOR.validate(request);