Refactor SAML attributes validation to follow standard patterns

This commit improves the SAML attributes validation by: 1. Adding a dedicated validate() method to SamlInitiateSingleSignOnAttributes that centralizes validation logic in one place 2. Moving validation from constructor to dedicated method for better error reporting 3. Checking both for null/empty keys and duplicate keys in the validate() method 4. Updating SamlInitiateSingleSignOnRequest to use the new validation method 5. Adding comprehensive tests for the new validation approach These changes follow standard Elasticsearch validation patterns, making the code more maintainable and consistent with the rest of the codebase.

Author: lloydmeta

x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java

@@ -15,8 +15,6 @@
 import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes;
 
 import java.io.IOException;
-import java.util.HashSet;
-import java.util.Set;
 
 import static org.elasticsearch.action.ValidateActions.addValidationError;
 
@@ -47,15 +45,12 @@ public ActionRequestValidationException validate() {
  validationException = addValidationError("acs is missing", validationException);
  }
 
- // Check for duplicate attribute keys
- if (attributes != null && attributes.getAttributes().isEmpty() == false) {
- Set<String> keys = new HashSet<>();
- for (SamlInitiateSingleSignOnAttributes.Attribute attribute : attributes.getAttributes()) {
- if (keys.add(attribute.getKey()) == false) {
- validationException = addValidationError(
- "duplicate attribute key [" + attribute.getKey() + "] found",
- validationException
- );
+ // Validate attributes if present
+ if (attributes != null) {
+ ActionRequestValidationException attributesValidationException = attributes.validate();
+ if (attributesValidationException != null) {
+ for (String error : attributesValidationException.validationErrors()) {
+ validationException = addValidationError(error, validationException);
  }
  }
  }

x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java

@@ -6,6 +6,8 @@
  */
 package org.elasticsearch.xpack.idp.saml.support;
 
+import org.elasticsearch.action.ActionRequestValidationException;
+import org.elasticsearch.action.ValidateActions;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -19,8 +21,10 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 
 /**
  * Represents a collection of SAML attributes to be included in the SAML response.
@@ -41,6 +45,33 @@ public SamlInitiateSingleSignOnAttributes(StreamInput in) throws IOException {
  }
  }
 
+ /**
+ * Validates this SAML attributes object to ensure all attribute keys are valid and unique.
+ * @return ActionRequestValidationException containing validation errors, or null if valid
+ */
+ public ActionRequestValidationException validate() {
+ ActionRequestValidationException validationException = null;
+
+ // Check for null/empty attribute keys and duplicate keys
+ if (attributes.isEmpty() == false) {
+ Set<String> keys = new HashSet<>();
+ for (Attribute attribute : attributes) {
+ // Check for null or empty key
+ if (Strings.isNullOrEmpty(attribute.getKey())) {
+ validationException = ValidateActions.addValidationError("attribute key cannot be null or empty", validationException);
+ } else if (keys.add(attribute.getKey()) == false) {
+ // Check for duplicate key
+ validationException = ValidateActions.addValidationError(
+ "duplicate attribute key [" + attribute.getKey() + "] found",
+ validationException
+ );
+ }
+ }
+ }
+
+ return validationException;
+ }
+
  public List<Attribute> getAttributes() {
  return Collections.unmodifiableList(attributes);
  }
@@ -117,12 +148,6 @@ public Attribute() {
  }
 
  public Attribute(String key, List<String> values) {
- if (key == null) {
- throw new IllegalArgumentException("Attribute key cannot be null");
- }
- if (key.isEmpty()) {
- throw new IllegalArgumentException("Attribute key cannot be empty");
- }
  this.key = key;
  this.values = new ArrayList<>(values);
  }

x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java

@@ -6,6 +6,7 @@
  */
 package org.elasticsearch.xpack.idp.saml.support;
 
+import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.test.ESTestCase;
@@ -232,18 +233,51 @@ public void testEqualsAndHashCode() {
  assertThat(attributes1.equals("string"), equalTo(false));
  }
 
- public void testIllegalArgumentHandling() {
- // Test null key handling
- expectThrows(
- IllegalArgumentException.class,
- () -> new SamlInitiateSingleSignOnAttributes.Attribute(null, Collections.singletonList("value"))
- );
-
- // Test empty key handling
- expectThrows(
- IllegalArgumentException.class,
- () -> new SamlInitiateSingleSignOnAttributes.Attribute("", Collections.singletonList("value"))
- );
+ public void testValidate() {
+ // Test with valid attributes - should pass validation
+ final SamlInitiateSingleSignOnAttributes validAttributes = new SamlInitiateSingleSignOnAttributes();
+ List<SamlInitiateSingleSignOnAttributes.Attribute> attributeList = new ArrayList<>();
+ attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Collections.singletonList("value1")));
+ attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Arrays.asList("value2A", "value2B")));
+ validAttributes.setAttributes(attributeList);
+
+ ActionRequestValidationException validationException = validAttributes.validate();
+ assertNull("Valid attributes should pass validation", validationException);
+
+ // Test with null key - should fail validation
+ final SamlInitiateSingleSignOnAttributes nullKeyAttributes = new SamlInitiateSingleSignOnAttributes();
+ attributeList = new ArrayList<>();
+ attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute(null, Collections.singletonList("value1")));
+ nullKeyAttributes.setAttributes(attributeList);
+
+ validationException = nullKeyAttributes.validate();
+ assertNotNull("Null key should fail validation", validationException);
+ assertThat(validationException.validationErrors().size(), equalTo(1));
+ assertThat(validationException.validationErrors().get(0), containsString("attribute key cannot be null or empty"));
+
+ // Test with empty key - should fail validation
+ final SamlInitiateSingleSignOnAttributes emptyKeyAttributes = new SamlInitiateSingleSignOnAttributes();
+ attributeList = new ArrayList<>();
+ attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("", Collections.singletonList("value1")));
+ emptyKeyAttributes.setAttributes(attributeList);
+
+ validationException = emptyKeyAttributes.validate();
+ assertNotNull("Empty key should fail validation", validationException);
+ assertThat(validationException.validationErrors().size(), equalTo(1));
+ assertThat(validationException.validationErrors().get(0), containsString("attribute key cannot be null or empty"));
+
+ // Test with duplicate keys - should fail validation
+ final SamlInitiateSingleSignOnAttributes duplicateKeyAttributes = new SamlInitiateSingleSignOnAttributes();
+ attributeList = new ArrayList<>();
+ attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("duplicate_key", Collections.singletonList("value1")));
+ attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("unique_key", Collections.singletonList("value2")));
+ attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("duplicate_key", Arrays.asList("value3", "value4")));
+ duplicateKeyAttributes.setAttributes(attributeList);
+
+ validationException = duplicateKeyAttributes.validate();
+ assertNotNull("Duplicate keys should fail validation", validationException);
+ assertThat(validationException.validationErrors().size(), equalTo(1));
+ assertThat(validationException.validationErrors().get(0), containsString("duplicate attribute key [duplicate_key] found"));
  }
 
  private SamlInitiateSingleSignOnAttributes parseFromJson(String json) throws IOException {