Skip to content

Commit 4b4b3ad

Browse files
lloydmetalloydmeta
committed
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.
1 parent a266115 commit 4b4b3ad

File tree

3 files changed

+83
-29
lines changed

3 files changed

+83
-29
lines changed

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

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515
import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes;
1616

1717
import java.io.IOException;
18-
import java.util.HashSet;
19-
import java.util.Set;
2018

2119
import static org.elasticsearch.action.ValidateActions.addValidationError;
2220

@@ -47,15 +45,12 @@ public ActionRequestValidationException validate() {
4745
validationException = addValidationError("acs is missing", validationException);
4846
}
4947

50-
// Check for duplicate attribute keys
51-
if (attributes != null && attributes.getAttributes().isEmpty() == false) {
52-
Set<String> keys = new HashSet<>();
53-
for (SamlInitiateSingleSignOnAttributes.Attribute attribute : attributes.getAttributes()) {
54-
if (keys.add(attribute.getKey()) == false) {
55-
validationException = addValidationError(
56-
"duplicate attribute key [" + attribute.getKey() + "] found",
57-
validationException
58-
);
48+
// Validate attributes if present
49+
if (attributes != null) {
50+
ActionRequestValidationException attributesValidationException = attributes.validate();
51+
if (attributesValidationException != null) {
52+
for (String error : attributesValidationException.validationErrors()) {
53+
validationException = addValidationError(error, validationException);
5954
}
6055
}
6156
}

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

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
*/
77
package org.elasticsearch.xpack.idp.saml.support;
88

9+
import org.elasticsearch.action.ActionRequestValidationException;
10+
import org.elasticsearch.action.ValidateActions;
911
import org.elasticsearch.common.Strings;
1012
import org.elasticsearch.common.io.stream.StreamInput;
1113
import org.elasticsearch.common.io.stream.StreamOutput;
@@ -19,8 +21,10 @@
1921
import java.io.IOException;
2022
import java.util.ArrayList;
2123
import java.util.Collections;
24+
import java.util.HashSet;
2225
import java.util.List;
2326
import java.util.Objects;
27+
import java.util.Set;
2428

2529
/**
2630
* Represents a collection of SAML attributes to be included in the SAML response.
@@ -41,6 +45,33 @@ public SamlInitiateSingleSignOnAttributes(StreamInput in) throws IOException {
4145
}
4246
}
4347

48+
/**
49+
* Validates this SAML attributes object to ensure all attribute keys are valid and unique.
50+
* @return ActionRequestValidationException containing validation errors, or null if valid
51+
*/
52+
public ActionRequestValidationException validate() {
53+
ActionRequestValidationException validationException = null;
54+
55+
// Check for null/empty attribute keys and duplicate keys
56+
if (attributes.isEmpty() == false) {
57+
Set<String> keys = new HashSet<>();
58+
for (Attribute attribute : attributes) {
59+
// Check for null or empty key
60+
if (Strings.isNullOrEmpty(attribute.getKey())) {
61+
validationException = ValidateActions.addValidationError("attribute key cannot be null or empty", validationException);
62+
} else if (keys.add(attribute.getKey()) == false) {
63+
// Check for duplicate key
64+
validationException = ValidateActions.addValidationError(
65+
"duplicate attribute key [" + attribute.getKey() + "] found",
66+
validationException
67+
);
68+
}
69+
}
70+
}
71+
72+
return validationException;
73+
}
74+
4475
public List<Attribute> getAttributes() {
4576
return Collections.unmodifiableList(attributes);
4677
}
@@ -117,12 +148,6 @@ public Attribute() {
117148
}
118149

119150
public Attribute(String key, List<String> values) {
120-
if (key == null) {
121-
throw new IllegalArgumentException("Attribute key cannot be null");
122-
}
123-
if (key.isEmpty()) {
124-
throw new IllegalArgumentException("Attribute key cannot be empty");
125-
}
126151
this.key = key;
127152
this.values = new ArrayList<>(values);
128153
}

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

Lines changed: 46 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
*/
77
package org.elasticsearch.xpack.idp.saml.support;
88

9+
import org.elasticsearch.action.ActionRequestValidationException;
910
import org.elasticsearch.common.Strings;
1011
import org.elasticsearch.common.io.stream.BytesStreamOutput;
1112
import org.elasticsearch.test.ESTestCase;
@@ -232,18 +233,51 @@ public void testEqualsAndHashCode() {
232233
assertThat(attributes1.equals("string"), equalTo(false));
233234
}
234235

235-
public void testIllegalArgumentHandling() {
236-
// Test null key handling
237-
expectThrows(
238-
IllegalArgumentException.class,
239-
() -> new SamlInitiateSingleSignOnAttributes.Attribute(null, Collections.singletonList("value"))
240-
);
241-
242-
// Test empty key handling
243-
expectThrows(
244-
IllegalArgumentException.class,
245-
() -> new SamlInitiateSingleSignOnAttributes.Attribute("", Collections.singletonList("value"))
246-
);
236+
public void testValidate() {
237+
// Test with valid attributes - should pass validation
238+
final SamlInitiateSingleSignOnAttributes validAttributes = new SamlInitiateSingleSignOnAttributes();
239+
List<SamlInitiateSingleSignOnAttributes.Attribute> attributeList = new ArrayList<>();
240+
attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Collections.singletonList("value1")));
241+
attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Arrays.asList("value2A", "value2B")));
242+
validAttributes.setAttributes(attributeList);
243+
244+
ActionRequestValidationException validationException = validAttributes.validate();
245+
assertNull("Valid attributes should pass validation", validationException);
246+
247+
// Test with null key - should fail validation
248+
final SamlInitiateSingleSignOnAttributes nullKeyAttributes = new SamlInitiateSingleSignOnAttributes();
249+
attributeList = new ArrayList<>();
250+
attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute(null, Collections.singletonList("value1")));
251+
nullKeyAttributes.setAttributes(attributeList);
252+
253+
validationException = nullKeyAttributes.validate();
254+
assertNotNull("Null key should fail validation", validationException);
255+
assertThat(validationException.validationErrors().size(), equalTo(1));
256+
assertThat(validationException.validationErrors().get(0), containsString("attribute key cannot be null or empty"));
257+
258+
// Test with empty key - should fail validation
259+
final SamlInitiateSingleSignOnAttributes emptyKeyAttributes = new SamlInitiateSingleSignOnAttributes();
260+
attributeList = new ArrayList<>();
261+
attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("", Collections.singletonList("value1")));
262+
emptyKeyAttributes.setAttributes(attributeList);
263+
264+
validationException = emptyKeyAttributes.validate();
265+
assertNotNull("Empty key should fail validation", validationException);
266+
assertThat(validationException.validationErrors().size(), equalTo(1));
267+
assertThat(validationException.validationErrors().get(0), containsString("attribute key cannot be null or empty"));
268+
269+
// Test with duplicate keys - should fail validation
270+
final SamlInitiateSingleSignOnAttributes duplicateKeyAttributes = new SamlInitiateSingleSignOnAttributes();
271+
attributeList = new ArrayList<>();
272+
attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("duplicate_key", Collections.singletonList("value1")));
273+
attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("unique_key", Collections.singletonList("value2")));
274+
attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("duplicate_key", Arrays.asList("value3", "value4")));
275+
duplicateKeyAttributes.setAttributes(attributeList);
276+
277+
validationException = duplicateKeyAttributes.validate();
278+
assertNotNull("Duplicate keys should fail validation", validationException);
279+
assertThat(validationException.validationErrors().size(), equalTo(1));
280+
assertThat(validationException.validationErrors().get(0), containsString("duplicate attribute key [duplicate_key] found"));
247281
}
248282

249283
private SamlInitiateSingleSignOnAttributes parseFromJson(String json) throws IOException {

0 commit comments

Comments
 (0)