From 505f064be3c9f4c1ad44471ee8bf97cdbd8b45a4 Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Tue, 20 May 2025 17:30:28 +0900 Subject: [PATCH 01/15] Implement SAML custom attributes support for Identity Provider This commit adds support for custom attributes in SAML single sign-on requests in the Elasticsearch X-Pack Identity Provider plugin. This feature allows passage of custom key-value attributes in SAML requests and responses. Key components: - Added SamlInitiateSingleSignOnAttributes class for holding attributes - Added validation for null and empty attribute keys - Updated request and response objects to handle attributes - Modified authentication flow to process attributes - Added test coverage to validate attributes functionality The implementation follows Elasticsearch patterns with robust validation and serialization mechanisms, while maintaining backward compatibility. --- .../idp/IdentityProviderAuthenticationIT.java | 81 +++++- .../SamlInitiateSingleSignOnRequest.java | 24 +- .../SamlInitiateSingleSignOnResponse.java | 19 ++ ...ansportSamlInitiateSingleSignOnAction.java | 2 +- ...lAuthenticationResponseMessageBuilder.java | 33 ++- .../RestSamlInitiateSingleSignOnAction.java | 6 + .../SamlInitiateSingleSignOnAttributes.java | 206 ++++++++++++++ ...enticationResponseMessageBuilderTests.java | 6 +- ...mlInitiateSingleSignOnAttributesTests.java | 264 ++++++++++++++++++ 9 files changed, 621 insertions(+), 20 deletions(-) create mode 100644 x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java create mode 100644 x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java diff --git a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java index c065e8d7e1d12..ee98940441b24 100644 --- a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java +++ b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java @@ -18,6 +18,7 @@ import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.core.Nullable; import org.elasticsearch.xcontent.ObjectPath; +import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.json.JsonXContent; import org.elasticsearch.xpack.core.security.action.saml.SamlPrepareAuthenticationResponse; import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex; @@ -25,6 +26,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Base64; import java.util.List; import java.util.Map; @@ -74,6 +76,47 @@ public void testRegistrationAndIdpInitiatedSso() throws Exception { authenticateWithSamlResponse(samlResponse, null); } + public void testCustomAttributesInIdpInitiatedSso() throws Exception { + final Map request = Map.ofEntries( + Map.entry("name", "Test SP With Custom Attributes"), + Map.entry("acs", SP_ACS), + Map.entry("privileges", Map.ofEntries(Map.entry("resource", SP_ENTITY_ID), Map.entry("roles", List.of("sso:(\\w+)")))), + Map.entry( + "attributes", + Map.ofEntries( + Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/principal"), + Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"), + Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"), + Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles") + ) + ) + ); + final SamlServiceProviderIndex.DocumentVersion docVersion = createServiceProvider(SP_ENTITY_ID, request); + checkIndexDoc(docVersion); + ensureGreen(SamlServiceProviderIndex.INDEX_NAME); + + // Create custom attributes + List> attributesList = new ArrayList<>(); + Map attr1 = Map.of("key", "department", "values", List.of("engineering", "product")); + Map attr2 = Map.of("key", "region", "values", List.of("APAC")); + attributesList.add(attr1); + attributesList.add(attr2); + + final Map requestAttributes = Map.of("attributes", attributesList); + + // Generate SAML response with custom attributes + final String samlResponse = generateSamlResponseWithAttributes(SP_ENTITY_ID, SP_ACS, null, requestAttributes); + + // Verify the response includes our custom attributes + assertThat(samlResponse, containsString("department")); + assertThat(samlResponse, containsString("engineering")); + assertThat(samlResponse, containsString("product")); + assertThat(samlResponse, containsString("region")); + assertThat(samlResponse, containsString("APAC")); + + authenticateWithSamlResponse(samlResponse, null); + } + public void testRegistrationAndSpInitiatedSso() throws Exception { final Map request = Map.ofEntries( Map.entry("name", "Test SP"), @@ -125,17 +168,37 @@ private SamlPrepareAuthenticationResponse generateSamlAuthnRequest(String realmN } } - private String generateSamlResponse(String entityId, String acs, @Nullable Map authnState) throws Exception { + private String generateSamlResponse(String entityId, String acs, @Nullable Map authnState) throws IOException { + return generateSamlResponseWithAttributes(entityId, acs, authnState, null); + } + + private String generateSamlResponseWithAttributes( + String entityId, + String acs, + @Nullable Map authnState, + @Nullable Map attributes + ) throws IOException { final Request request = new Request("POST", "/_idp/saml/init"); - if (authnState != null && authnState.isEmpty() == false) { - request.setJsonEntity(Strings.format(""" - {"entity_id":"%s", "acs":"%s","authn_state":%s} - """, entityId, acs, Strings.toString(JsonXContent.contentBuilder().map(authnState)))); - } else { - request.setJsonEntity(Strings.format(""" - {"entity_id":"%s", "acs":"%s"} - """, entityId, acs)); + + XContentBuilder builder = JsonXContent.contentBuilder(); + builder.startObject(); + builder.field("entity_id", entityId); + builder.field("acs", acs); + + if (authnState != null) { + builder.field("authn_state"); + builder.map(authnState); } + + if (attributes != null) { + builder.field("attributes"); + builder.map(attributes); + } + + builder.endObject(); + String jsonEntity = Strings.toString(builder); + + request.setJsonEntity(jsonEntity); request.setOptions( RequestOptions.DEFAULT.toBuilder() .addHeader("es-secondary-authorization", basicAuthHeaderValue("idp_user", new SecureString("idp-password".toCharArray()))) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java index 6070b247093e1..406deb7911493 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java @@ -12,6 +12,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.xpack.idp.saml.support.SamlAuthenticationState; +import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; import java.io.IOException; @@ -22,12 +23,14 @@ public class SamlInitiateSingleSignOnRequest extends LegacyActionRequest { private String spEntityId; private String assertionConsumerService; private SamlAuthenticationState samlAuthenticationState; + private SamlInitiateSingleSignOnAttributes attributes; public SamlInitiateSingleSignOnRequest(StreamInput in) throws IOException { super(in); spEntityId = in.readString(); assertionConsumerService = in.readString(); samlAuthenticationState = in.readOptionalWriteable(SamlAuthenticationState::new); + attributes = in.readOptionalWriteable(SamlInitiateSingleSignOnAttributes::new); } public SamlInitiateSingleSignOnRequest() {} @@ -68,17 +71,36 @@ public void setSamlAuthenticationState(SamlAuthenticationState samlAuthenticatio this.samlAuthenticationState = samlAuthenticationState; } + public SamlInitiateSingleSignOnAttributes getAttributes() { + return attributes; + } + + public void setAttributes(SamlInitiateSingleSignOnAttributes attributes) { + this.attributes = attributes; + } + @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); out.writeString(spEntityId); out.writeString(assertionConsumerService); out.writeOptionalWriteable(samlAuthenticationState); + out.writeOptionalWriteable(attributes); } @Override public String toString() { - return getClass().getSimpleName() + "{spEntityId='" + spEntityId + "', acs='" + assertionConsumerService + "'}"; + return getClass().getSimpleName() + + "{" + + "spEntityId='" + + spEntityId + + "', " + + "acs='" + + assertionConsumerService + + "', " + + "attributes=" + + attributes + + "'}"; } } diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnResponse.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnResponse.java index c0a5157557f58..ba3ca1df9d177 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnResponse.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnResponse.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.core.Nullable; import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; import java.io.IOException; @@ -20,6 +21,7 @@ public class SamlInitiateSingleSignOnResponse extends ActionResponse { private final String entityId; private final String samlStatus; private final String error; + private final SamlInitiateSingleSignOnAttributes attributes; public SamlInitiateSingleSignOnResponse( String entityId, @@ -27,12 +29,24 @@ public SamlInitiateSingleSignOnResponse( String samlResponse, String samlStatus, @Nullable String error + ) { + this(entityId, postUrl, samlResponse, samlStatus, error, null); + } + + public SamlInitiateSingleSignOnResponse( + String entityId, + String postUrl, + String samlResponse, + String samlStatus, + @Nullable String error, + @Nullable SamlInitiateSingleSignOnAttributes attributes ) { this.entityId = entityId; this.postUrl = postUrl; this.samlResponse = samlResponse; this.samlStatus = samlStatus; this.error = error; + this.attributes = attributes; } public String getPostUrl() { @@ -55,6 +69,10 @@ public String getSamlStatus() { return samlStatus; } + public SamlInitiateSingleSignOnAttributes getAttributes() { + return attributes; + } + @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(entityId); @@ -62,6 +80,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeString(samlResponse); out.writeString(samlStatus); out.writeOptionalString(error); + out.writeOptionalWriteable(attributes); } public void toXContent(XContentBuilder builder) throws IOException { diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/TransportSamlInitiateSingleSignOnAction.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/TransportSamlInitiateSingleSignOnAction.java index 07d71481326d4..ea75e68506773 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/TransportSamlInitiateSingleSignOnAction.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/TransportSamlInitiateSingleSignOnAction.java @@ -139,7 +139,7 @@ protected void doExecute( identityProvider ); try { - final Response response = builder.build(user, authenticationState); + final Response response = builder.build(user, authenticationState, request.getAttributes()); listener.onResponse( new SamlInitiateSingleSignOnResponse( user.getServiceProvider().getEntityId(), diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java index 5d8cbf2607338..4ac44e9285a84 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java @@ -18,6 +18,7 @@ import org.elasticsearch.xpack.idp.saml.support.SamlAuthenticationState; import org.elasticsearch.xpack.idp.saml.support.SamlFactory; import org.elasticsearch.xpack.idp.saml.support.SamlInit; +import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; import org.elasticsearch.xpack.idp.saml.support.SamlObjectSigner; import org.opensaml.core.xml.schema.XSString; import org.opensaml.saml.saml2.core.Assertion; @@ -66,7 +67,11 @@ public SuccessfulAuthenticationResponseMessageBuilder(SamlFactory samlFactory, C this.idp = idp; } - public Response build(UserServiceAuthentication user, @Nullable SamlAuthenticationState authnState) { + public Response build( + UserServiceAuthentication user, + @Nullable SamlAuthenticationState authnState, + @Nullable SamlInitiateSingleSignOnAttributes customAttributes + ) { logger.debug("Building success response for [{}] from [{}]", user, authnState); final Instant now = clock.instant(); final SamlServiceProvider serviceProvider = user.getServiceProvider(); @@ -87,10 +92,13 @@ public Response build(UserServiceAuthentication user, @Nullable SamlAuthenticati assertion.setIssueInstant(now); assertion.setConditions(buildConditions(now, serviceProvider)); assertion.setSubject(buildSubject(now, user, authnState)); - assertion.getAuthnStatements().add(buildAuthnStatement(now, user)); - final AttributeStatement attributes = buildAttributes(user); - if (attributes != null) { - assertion.getAttributeStatements().add(attributes); + + final AuthnStatement authnStatement = buildAuthnStatement(now, user); + assertion.getAuthnStatements().add(authnStatement); + + final AttributeStatement attributeStatement = buildAttributes(user, customAttributes); + if (attributeStatement != null) { + assertion.getAttributeStatements().add(attributeStatement); } response.getAssertions().add(assertion); return sign(response); @@ -179,7 +187,10 @@ private static String resolveAuthnClass(Set authentication } } - private AttributeStatement buildAttributes(UserServiceAuthentication user) { + private AttributeStatement buildAttributes( + UserServiceAuthentication user, + @Nullable SamlInitiateSingleSignOnAttributes customAttributes + ) { final SamlServiceProvider serviceProvider = user.getServiceProvider(); final AttributeStatement statement = samlFactory.object(AttributeStatement.class, AttributeStatement.DEFAULT_ELEMENT_NAME); final List attributes = new ArrayList<>(); @@ -199,6 +210,16 @@ private AttributeStatement buildAttributes(UserServiceAuthentication user) { if (name != null) { attributes.add(name); } + // Add custom attributes if provided + if (customAttributes != null && customAttributes.getAttributes().isEmpty() == false) { + for (SamlInitiateSingleSignOnAttributes.Attribute customAttr : customAttributes.getAttributes()) { + Attribute attribute = buildAttribute(customAttr.getKey(), customAttr.getKey(), customAttr.getValues()); + if (attribute != null) { + attributes.add(attribute); + } + } + } + if (attributes.isEmpty()) { return null; } diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/rest/action/RestSamlInitiateSingleSignOnAction.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/rest/action/RestSamlInitiateSingleSignOnAction.java index 3e4d57860fdae..5170254411dd9 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/rest/action/RestSamlInitiateSingleSignOnAction.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/rest/action/RestSamlInitiateSingleSignOnAction.java @@ -20,6 +20,7 @@ import org.elasticsearch.xpack.idp.action.SamlInitiateSingleSignOnRequest; import org.elasticsearch.xpack.idp.action.SamlInitiateSingleSignOnResponse; import org.elasticsearch.xpack.idp.saml.support.SamlAuthenticationState; +import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; import java.io.IOException; import java.util.Collections; @@ -41,6 +42,11 @@ public class RestSamlInitiateSingleSignOnAction extends IdpBaseRestHandler { (p, c) -> SamlAuthenticationState.fromXContent(p), new ParseField("authn_state") ); + PARSER.declareObject( + SamlInitiateSingleSignOnRequest::setAttributes, + (p, c) -> SamlInitiateSingleSignOnAttributes.fromXContent(p), + new ParseField("attributes") + ); } public RestSamlInitiateSingleSignOnAction(XPackLicenseState licenseState) { diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java new file mode 100644 index 0000000000000..7a946a0b98554 --- /dev/null +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java @@ -0,0 +1,206 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ +package org.elasticsearch.xpack.idp.saml.support; + +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.xcontent.ObjectParser; +import org.elasticsearch.xcontent.ParseField; +import org.elasticsearch.xcontent.ToXContentObject; +import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xcontent.XContentParser; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * Represents a collection of SAML attributes to be included in the SAML response. + * Each attribute has a key and a list of values. + */ +public class SamlInitiateSingleSignOnAttributes implements Writeable, ToXContentObject { + private List attributes; + + public SamlInitiateSingleSignOnAttributes() { + this.attributes = new ArrayList<>(); + } + + public SamlInitiateSingleSignOnAttributes(StreamInput in) throws IOException { + int size = in.readVInt(); + attributes = new ArrayList<>(size); + for (int i = 0; i < size; i++) { + attributes.add(new Attribute(in)); + } + } + + public List getAttributes() { + return Collections.unmodifiableList(attributes); + } + + public void setAttributes(List attributes) { + this.attributes = new ArrayList<>(attributes); + } + + public static final ObjectParser PARSER = new ObjectParser<>( + "saml_attributes", + true, + SamlInitiateSingleSignOnAttributes::new + ); + + static { + PARSER.declareObjectArray(SamlInitiateSingleSignOnAttributes::setAttributes, Attribute.PARSER, Fields.ATTRIBUTES); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(); + builder.startArray(Fields.ATTRIBUTES.getPreferredName()); + for (Attribute attribute : attributes) { + attribute.toXContent(builder, params); + } + builder.endArray(); + return builder.endObject(); + } + + public static SamlInitiateSingleSignOnAttributes fromXContent(XContentParser parser) throws IOException { + SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); + return PARSER.parse(parser, attributes, null); + } + + public interface Fields { + ParseField ATTRIBUTES = new ParseField("attributes"); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeVInt(attributes.size()); + for (Attribute attribute : attributes) { + attribute.writeTo(out); + } + } + + @Override + public String toString() { + return getClass().getSimpleName() + "{attributes=" + attributes + "}"; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + SamlInitiateSingleSignOnAttributes that = (SamlInitiateSingleSignOnAttributes) o; + return Objects.equals(attributes, that.attributes); + } + + @Override + public int hashCode() { + return Objects.hash(attributes); + } + + /** + * Individual attribute with a key and a list of values. + */ + public static class Attribute implements Writeable, ToXContentObject { + private String key; + private List values; + + public Attribute() { + this.values = new ArrayList<>(); + } + + public Attribute(String key, List 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); + } + + public Attribute(StreamInput in) throws IOException { + this.key = in.readString(); + int size = in.readVInt(); + this.values = new ArrayList<>(size); + for (int i = 0; i < size; i++) { + values.add(in.readString()); + } + } + + public String getKey() { + return key; + } + + public void setKey(String key) { + this.key = key; + } + + public List getValues() { + return Collections.unmodifiableList(values); + } + + public void setValues(List values) { + this.values = new ArrayList<>(values); + } + + public static final ObjectParser PARSER = new ObjectParser<>("attribute", true, Attribute::new); + + static { + PARSER.declareString(Attribute::setKey, AttributeFields.KEY); + PARSER.declareStringArray(Attribute::setValues, AttributeFields.VALUES); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(); + builder.field(AttributeFields.KEY.getPreferredName(), key); + builder.startArray(AttributeFields.VALUES.getPreferredName()); + for (String value : values) { + builder.value(value); + } + builder.endArray(); + return builder.endObject(); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeString(key); + out.writeVInt(values.size()); + for (String value : values) { + out.writeString(value); + } + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Attribute attribute = (Attribute) o; + return Objects.equals(key, attribute.key) && Objects.equals(values, attribute.values); + } + + @Override + public int hashCode() { + return Objects.hash(key, values); + } + + @Override + public String toString() { + return getClass().getSimpleName() + Strings.toString(this); + } + + public interface AttributeFields { + ParseField KEY = new ParseField("key"); + ParseField VALUES = new ParseField("values"); + } + } +} diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java index bd433e828436b..8f4b51965739d 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java @@ -17,7 +17,7 @@ import org.junit.Before; import org.opensaml.saml.saml2.core.Response; -import java.net.URL; +import java.net.URI; import java.time.Clock; import java.time.Duration; import java.util.Set; @@ -59,7 +59,7 @@ private Response buildResponse() throws Exception { final String baseServiceUrl = "https://" + randomAlphaOfLength(32) + ".us-east-1.aws.found.io/"; final String acs = baseServiceUrl + "api/security/saml/callback"; when(sp.getEntityId()).thenReturn(baseServiceUrl); - when(sp.getAssertionConsumerService()).thenReturn(new URL(acs)); + when(sp.getAssertionConsumerService()).thenReturn(URI.create(acs).toURL()); when(sp.getAuthnExpiry()).thenReturn(Duration.ofMinutes(10)); when(sp.getAttributeNames()).thenReturn(new SamlServiceProvider.AttributeNames("principal", null, null, null)); @@ -75,7 +75,7 @@ private Response buildResponse() throws Exception { clock, idp ); - return builder.build(user, null); + return builder.build(user, null, null); } } diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java new file mode 100644 index 0000000000000..eddc079e77802 --- /dev/null +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java @@ -0,0 +1,264 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ +package org.elasticsearch.xpack.idp.saml.support; + +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xcontent.ToXContent; +import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xcontent.XContentFactory; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentType; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.not; + +public class SamlInitiateSingleSignOnAttributesTests extends ESTestCase { + + public void testConstructors() { + // Test default constructor + final SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(); + assertThat(attributes1.getAttributes(), hasSize(0)); + + // Test with empty attribute list + final SamlInitiateSingleSignOnAttributes attributes2 = new SamlInitiateSingleSignOnAttributes(); + attributes2.setAttributes(Collections.emptyList()); + assertThat(attributes2.getAttributes(), hasSize(0)); + + // Test with non-empty attribute list + List attributeList = new ArrayList<>(); + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Collections.singletonList("value1"))); + final SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(); + attributes3.setAttributes(attributeList); + assertThat(attributes3.getAttributes(), hasSize(1)); + assertThat(attributes3.getAttributes().get(0).getKey(), equalTo("key1")); + } + + public void testEmptyAttributes() throws IOException { + final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); + assertThat(attributes.getAttributes(), hasSize(0)); + + final XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON); + attributes.toXContent(builder, ToXContent.EMPTY_PARAMS); + final String json = Strings.toString(builder); + assertThat(json, equalTo("{\"attributes\":[]}")); + + final SamlInitiateSingleSignOnAttributes parsedAttributes = parseFromJson(json); + assertThat(parsedAttributes.getAttributes(), hasSize(0)); + + // Test wire serialization + SamlInitiateSingleSignOnAttributes serialized = copySerialize(attributes); + assertThat(serialized.getAttributes(), hasSize(0)); + } + + public void testWithAttributes() throws IOException { + final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); + + List attributeList = new ArrayList<>(); + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "value2"))); + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Collections.singletonList("value3"))); + + attributes.setAttributes(attributeList); + + assertThat(attributes.getAttributes(), hasSize(2)); + assertThat(attributes.getAttributes().get(0).getKey(), equalTo("key1")); + assertThat(attributes.getAttributes().get(0).getValues(), hasSize(2)); + assertThat(attributes.getAttributes().get(0).getValues().get(0), equalTo("value1")); + assertThat(attributes.getAttributes().get(0).getValues().get(1), equalTo("value2")); + + assertThat(attributes.getAttributes().get(1).getKey(), equalTo("key2")); + assertThat(attributes.getAttributes().get(1).getValues(), hasSize(1)); + assertThat(attributes.getAttributes().get(1).getValues().get(0), equalTo("value3")); + + final XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON); + attributes.toXContent(builder, ToXContent.EMPTY_PARAMS); + final String json = Strings.toString(builder); + + // Check that JSON contains expected values + assertThat(json, containsString("key1")); + assertThat(json, containsString("value1")); + assertThat(json, containsString("value2")); + assertThat(json, containsString("key2")); + assertThat(json, containsString("value3")); + + // Check that parsed JSON matches original attributes + final SamlInitiateSingleSignOnAttributes parsedAttributes = parseFromJson(json); + assertThat(parsedAttributes.getAttributes(), hasSize(2)); + assertThat(parsedAttributes.getAttributes().get(0).getKey(), equalTo("key1")); + assertThat(parsedAttributes.getAttributes().get(0).getValues(), hasSize(2)); + assertThat(parsedAttributes.getAttributes().get(1).getKey(), equalTo("key2")); + assertThat(parsedAttributes.getAttributes().get(1).getValues(), hasSize(1)); + + // Test equals/hashCode + assertThat(attributes, equalTo(parsedAttributes)); + assertThat(attributes.hashCode(), equalTo(parsedAttributes.hashCode())); + + // Test wire serialization + SamlInitiateSingleSignOnAttributes serialized = copySerialize(attributes); + assertThat(serialized.getAttributes(), hasSize(2)); + assertThat(serialized.getAttributes().get(0).getKey(), equalTo("key1")); + assertThat(serialized.getAttributes().get(0).getValues(), hasSize(2)); + assertThat(serialized.getAttributes().get(1).getKey(), equalTo("key2")); + assertThat(serialized.getAttributes().get(1).getValues(), hasSize(1)); + } + + public void testToString() { + final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); + List attributeList = new ArrayList<>(); + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "value2"))); + attributes.setAttributes(attributeList); + + String toString = attributes.toString(); + assertThat(toString, containsString("SamlInitiateSingleSignOnAttributes")); + assertThat(toString, containsString("key1")); + assertThat(toString, containsString("value1")); + assertThat(toString, containsString("value2")); + + // Test with multiple attributes + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Collections.singletonList("value3"))); + attributes.setAttributes(attributeList); + toString = attributes.toString(); + assertThat(toString, containsString("key1")); + assertThat(toString, containsString("key2")); + assertThat(toString, containsString("value3")); + + // Test with empty attributes + final SamlInitiateSingleSignOnAttributes emptyAttributes = new SamlInitiateSingleSignOnAttributes(); + toString = emptyAttributes.toString(); + assertThat(toString, containsString("SamlInitiateSingleSignOnAttributes")); + assertThat(toString, containsString("attributes=[]")); + } + + public void testAttributeClass() throws IOException { + SamlInitiateSingleSignOnAttributes.Attribute attribute = new SamlInitiateSingleSignOnAttributes.Attribute( + "testKey", + Arrays.asList("val1", "val2") + ); + + assertThat(attribute.getKey(), equalTo("testKey")); + assertThat(attribute.getValues(), hasSize(2)); + assertThat(attribute.getValues().get(0), equalTo("val1")); + assertThat(attribute.getValues().get(1), equalTo("val2")); + + // Test equals/hashCode + SamlInitiateSingleSignOnAttributes.Attribute attribute2 = new SamlInitiateSingleSignOnAttributes.Attribute( + "testKey", + Arrays.asList("val1", "val2") + ); + assertThat(attribute, equalTo(attribute2)); + assertThat(attribute.hashCode(), equalTo(attribute2.hashCode())); + + SamlInitiateSingleSignOnAttributes.Attribute attribute3 = new SamlInitiateSingleSignOnAttributes.Attribute( + "differentKey", + Arrays.asList("val1", "val2") + ); + assertThat(attribute, not(equalTo(attribute3))); + + SamlInitiateSingleSignOnAttributes.Attribute attribute4 = new SamlInitiateSingleSignOnAttributes.Attribute( + "testKey", + Arrays.asList("differentValue") + ); + assertThat(attribute, not(equalTo(attribute4))); + + // Test equals with different object types + assertThat(attribute.equals("someString"), equalTo(false)); + assertThat(attribute.equals(null), equalTo(false)); + + // Test setter methods + SamlInitiateSingleSignOnAttributes.Attribute mutableAttr = new SamlInitiateSingleSignOnAttributes.Attribute( + "testKey", + Collections.emptyList() + ); + mutableAttr.setKey("newKey"); + mutableAttr.setValues(Arrays.asList("newVal1", "newVal2")); + assertThat(mutableAttr.getKey(), equalTo("newKey")); + assertThat(mutableAttr.getValues(), hasSize(2)); + assertThat(mutableAttr.getValues().get(0), equalTo("newVal1")); + assertThat(mutableAttr.getValues().get(1), equalTo("newVal2")); + + // Test toString + String toString = attribute.toString(); + assertThat(toString, containsString("Attribute")); + assertThat(toString, containsString("testKey")); + assertThat(toString, containsString("val1")); + } + + public void testEqualsAndHashCode() { + // Create attributes objects with the same content + List attributeList1 = new ArrayList<>(); + attributeList1.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "value2"))); + attributeList1.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Collections.singletonList("value3"))); + + SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(); + attributes1.setAttributes(attributeList1); + + List attributeList2 = new ArrayList<>(); + attributeList2.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "value2"))); + attributeList2.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Collections.singletonList("value3"))); + + SamlInitiateSingleSignOnAttributes attributes2 = new SamlInitiateSingleSignOnAttributes(); + attributes2.setAttributes(attributeList2); + + // Test equals and hashCode for identical objects + assertThat(attributes1, equalTo(attributes2)); + assertThat(attributes1.hashCode(), equalTo(attributes2.hashCode())); + + // Test equals with self + assertThat(attributes1.equals(attributes1), equalTo(true)); + + // Test different objects + List attributeList3 = new ArrayList<>(); + attributeList3.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "different"))); + SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(); + attributes3.setAttributes(attributeList3); + + assertThat(attributes1.equals(attributes3), equalTo(false)); + + // Test equals with null and different object types + assertThat(attributes1.equals(null), equalTo(false)); + 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")) + ); + } + + private SamlInitiateSingleSignOnAttributes parseFromJson(String json) throws IOException { + try (XContentParser parser = createParser(XContentType.JSON.xContent(), json)) { + parser.nextToken(); // Start object + return SamlInitiateSingleSignOnAttributes.fromXContent(parser); + } + } + + private SamlInitiateSingleSignOnAttributes copySerialize(SamlInitiateSingleSignOnAttributes attributes) throws IOException { + try (BytesStreamOutput out = new BytesStreamOutput()) { + attributes.writeTo(out); + try (var in = out.bytes().streamInput()) { + return new SamlInitiateSingleSignOnAttributes(in); + } + } + } +} From 763363bd87cff2e377bb505d514d3b55806b77da Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Tue, 20 May 2025 17:41:53 +0900 Subject: [PATCH 02/15] Add test for SAML custom attributes in authentication response This commit adds a comprehensive test that verifies SAML custom attributes are correctly handled in the authentication response builder. The test ensures: 1. Custom attributes with single and multiple values are properly included 2. The response with custom attributes is still correctly signed 3. The XML schema validation still passes with custom attributes 4. We can locate and verify individual attribute values in the response This provides critical test coverage for the SAML custom attributes feature implementation. --- ...enticationResponseMessageBuilderTests.java | 57 ++++++++++++++++++- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java index 8f4b51965739d..a91c344748d47 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java @@ -12,14 +12,20 @@ import org.elasticsearch.xpack.idp.saml.sp.ServiceProviderDefaults; import org.elasticsearch.xpack.idp.saml.support.SamlFactory; import org.elasticsearch.xpack.idp.saml.support.SamlInit; +import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; import org.elasticsearch.xpack.idp.saml.support.XmlValidator; import org.elasticsearch.xpack.idp.saml.test.IdpSamlTestCase; import org.junit.Before; +import org.opensaml.saml.saml2.core.Attribute; +import org.opensaml.saml.saml2.core.AttributeStatement; import org.opensaml.saml.saml2.core.Response; import java.net.URI; import java.time.Clock; import java.time.Duration; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import java.util.Set; import static org.hamcrest.Matchers.containsString; @@ -46,13 +52,58 @@ public void setupSaml() throws Exception { } public void testSignedResponseIsValidAgainstXmlSchema() throws Exception { - final Response response = buildResponse(); + final Response response = buildResponse(null); final String xml = super.toString(response); assertThat(xml, containsString("SignedInfo>")); validator.validate(xml); } - private Response buildResponse() throws Exception { + public void testSignedResponseWithCustomAttributes() throws Exception { + // Create custom attributes + SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); + List attributeList = new ArrayList<>(); + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("customAttr1", Collections.singletonList("value1"))); + + List multipleValues = new ArrayList<>(); + multipleValues.add("value2A"); + multipleValues.add("value2B"); + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("customAttr2", multipleValues)); + + attributes.setAttributes(attributeList); + + // Build response with custom attributes + final Response response = buildResponse(attributes); + final String xml = super.toString(response); + + // Validate that response is correctly signed + assertThat(xml, containsString("SignedInfo>")); + validator.validate(xml); + + // Verify custom attributes are included + boolean foundCustomAttr1 = false; + boolean foundCustomAttr2 = false; + + for (AttributeStatement statement : response.getAssertions().get(0).getAttributeStatements()) { + for (Attribute attribute : statement.getAttributes()) { + String name = attribute.getName(); + if (name.equals("customAttr1")) { + foundCustomAttr1 = true; + assertEquals(1, attribute.getAttributeValues().size()); + assertThat(attribute.getAttributeValues().get(0).getDOM().getTextContent(), containsString("value1")); + } else if (name.equals("customAttr2")) { + foundCustomAttr2 = true; + assertEquals(2, attribute.getAttributeValues().size()); + assertThat(attribute.getAttributeValues().get(0).getDOM().getTextContent(), containsString("value2A")); + assertThat(attribute.getAttributeValues().get(1).getDOM().getTextContent(), containsString("value2B")); + } + } + } + + assertTrue("Custom attribute 'customAttr1' not found in SAML response", foundCustomAttr1); + assertTrue("Custom attribute 'customAttr2' not found in SAML response", foundCustomAttr2); + } + + private Response buildResponse(SamlInitiateSingleSignOnAttributes customAttributes) throws Exception { final Clock clock = Clock.systemUTC(); final SamlServiceProvider sp = mock(SamlServiceProvider.class); @@ -75,7 +126,7 @@ private Response buildResponse() throws Exception { clock, idp ); - return builder.build(user, null, null); + return builder.build(user, null, customAttributes); } } From 67d94ea6ac71582591c54ff9415d801be4e8f0ad Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Tue, 20 May 2025 18:33:30 +0900 Subject: [PATCH 03/15] Add backward compatibility overload for SuccessfulAuthenticationResponseMessageBuilder.build This commit adds an overloaded build method that accepts only two parameters (user and authenticationState) and forwards the call to the three-parameter version with null for the customAttributes parameter. This maintains backward compatibility with existing code that doesn't use custom attributes. This fixes a compilation error in ServerlessSsoIT.java which was still using the two-parameter method signature. Signed-off-by: lloydmeta --- ...lAuthenticationResponseMessageBuilder.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java index 4ac44e9285a84..2be14cff03b21 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java @@ -67,6 +67,25 @@ public SuccessfulAuthenticationResponseMessageBuilder(SamlFactory samlFactory, C this.idp = idp; } + /** + * Builds and signs a SAML Response Message with a single assertion for the provided user + * + * @param user The user who is authenticated (actually a combination of user+sp) + * @param authnState The authentication state as presented in the SAML request (or {@code null}) + * @return A SAML Response + */ + public Response build(UserServiceAuthentication user, @Nullable SamlAuthenticationState authnState) { + return build(user, authnState, null); + } + + /** + * Builds and signs a SAML Response Message with a single assertion for the provided user + * + * @param user The user who is authenticated (actually a combination of user+sp) + * @param authnState The authentication state as presented in the SAML request (or {@code null}) + * @param customAttributes Optional custom attributes to include in the response (or {@code null}) + * @return A SAML Response + */ public Response build( UserServiceAuthentication user, @Nullable SamlAuthenticationState authnState, From a2661153e4ce1d4b11833e25e85560fa6c739958 Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Tue, 20 May 2025 19:53:18 +0900 Subject: [PATCH 04/15] Add validation for duplicate SAML attribute keys This commit enhances the SAML attributes implementation by adding validation for duplicate attribute keys. When the same attribute key appears multiple times in a request, the validation will now fail with a clear error message. Signed-off-by: lloydmeta --- .../SamlInitiateSingleSignOnRequest.java | 16 ++++++++ .../SamlInitiateSingleSignOnRequestTests.java | 40 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java index 406deb7911493..bff36eb5c9dbc 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java @@ -15,6 +15,8 @@ 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; @@ -44,6 +46,20 @@ public ActionRequestValidationException validate() { if (Strings.isNullOrEmpty(assertionConsumerService)) { validationException = addValidationError("acs is missing", validationException); } + + // Check for duplicate attribute keys + if (attributes != null && attributes.getAttributes().isEmpty() == false) { + Set keys = new HashSet<>(); + for (SamlInitiateSingleSignOnAttributes.Attribute attribute : attributes.getAttributes()) { + if (keys.add(attribute.getKey()) == false) { + validationException = addValidationError( + "duplicate attribute key [" + attribute.getKey() + "] found", + validationException + ); + } + } + } + return validationException; } diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java index f930faffd40c7..85bd6aa0e1ca7 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java @@ -9,6 +9,12 @@ import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; @@ -39,4 +45,38 @@ public void testValidation() { assertThat(validationException.validationErrors().get(0), containsString("entity_id is missing")); assertThat(validationException.validationErrors().get(1), containsString("acs is missing")); } + + public void testDuplicateAttributeKeysValidation() { + // Create request with valid required fields + final SamlInitiateSingleSignOnRequest request = new SamlInitiateSingleSignOnRequest(); + request.setSpEntityId("https://kibana_url"); + request.setAssertionConsumerService("https://kibana_url/acs"); + + // Test with unique attribute keys - should be valid + SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); + List attributeList = new ArrayList<>(); + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Collections.singletonList("value1"))); + attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Arrays.asList("value2A", "value2B"))); + attributes.setAttributes(attributeList); + request.setAttributes(attributes); + + // Should pass validation + ActionRequestValidationException validationException = request.validate(); + assertNull("Request with unique attribute keys should pass validation", validationException); + + // Test with duplicate attribute keys - should be invalid + attributes = 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"))); + attributes.setAttributes(attributeList); + request.setAttributes(attributes); + + // Should fail validation with appropriate error message + validationException = request.validate(); + assertNotNull("Request with duplicate attribute keys should fail validation", validationException); + assertThat(validationException.validationErrors().size(), equalTo(1)); + assertThat(validationException.validationErrors().get(0), containsString("duplicate attribute key [duplicate_key] found")); + } } From 4b4b3ad29e34a1b6b40b65dbb78c5d15620abba9 Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Wed, 21 May 2025 11:48:01 +0900 Subject: [PATCH 05/15] 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. --- .../SamlInitiateSingleSignOnRequest.java | 17 ++---- .../SamlInitiateSingleSignOnAttributes.java | 37 ++++++++++-- ...mlInitiateSingleSignOnAttributesTests.java | 58 +++++++++++++++---- 3 files changed, 83 insertions(+), 29 deletions(-) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java index bff36eb5c9dbc..882da7c5632ed 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java +++ b/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 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); } } } diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java index 7a946a0b98554..fa0d4b3de669a 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java +++ b/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 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 getAttributes() { return Collections.unmodifiableList(attributes); } @@ -117,12 +148,6 @@ public Attribute() { } public Attribute(String key, List 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); } diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java index eddc079e77802..ba4bb2edc881c 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java +++ b/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 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 { From 4114c68be116e2f858cac7aeea00d596b8292cc2 Mon Sep 17 00:00:00 2001 From: Lloyd Date: Wed, 21 May 2025 12:15:06 +0900 Subject: [PATCH 06/15] Update docs/changelog/128176.yaml --- docs/changelog/128176.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/128176.yaml diff --git a/docs/changelog/128176.yaml b/docs/changelog/128176.yaml new file mode 100644 index 0000000000000..2cf76c4513772 --- /dev/null +++ b/docs/changelog/128176.yaml @@ -0,0 +1,5 @@ +pr: 128176 +summary: Implement SAML custom attributes support for Identity Provider +area: Authentication +type: enhancement +issues: [] From ae2152957249fb34fcf057cdd5ad028b7e719145 Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Wed, 21 May 2025 14:55:25 +0900 Subject: [PATCH 07/15] Improve SAML response validation in identity provider tests Enhanced the testCustomAttributesInIdpInitiatedSso test to properly validate both SAML response structure and custom attributes using DOM parsing and XPath. Key improvements: - Validate SAML Response/Assertion elements exist - Precisely validate custom attributes (department, region) and their values - Use namespace-aware XML parsing for resilience to format changes Signed-off-by: lloydmeta --- .../idp/IdentityProviderAuthenticationIT.java | 67 +++++++++++++++++-- 1 file changed, 60 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java index ee98940441b24..80f8c498f8632 100644 --- a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java +++ b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java @@ -23,8 +23,13 @@ import org.elasticsearch.xpack.core.security.action.saml.SamlPrepareAuthenticationResponse; import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProviderIndex; import org.junit.Before; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.NodeList; +import org.xml.sax.InputSource; import java.io.IOException; +import java.io.StringReader; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Base64; @@ -32,11 +37,19 @@ import java.util.Map; import java.util.Set; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.xpath.XPath; +import javax.xml.xpath.XPathConstants; +import javax.xml.xpath.XPathFactory; + import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; public class IdentityProviderAuthenticationIT extends IdpRestTestCase { @@ -98,7 +111,7 @@ public void testCustomAttributesInIdpInitiatedSso() throws Exception { // Create custom attributes List> attributesList = new ArrayList<>(); Map attr1 = Map.of("key", "department", "values", List.of("engineering", "product")); - Map attr2 = Map.of("key", "region", "values", List.of("APAC")); + Map attr2 = Map.of("key", "region", "values", List.of("APJ")); attributesList.add(attr1); attributesList.add(attr2); @@ -107,12 +120,52 @@ public void testCustomAttributesInIdpInitiatedSso() throws Exception { // Generate SAML response with custom attributes final String samlResponse = generateSamlResponseWithAttributes(SP_ENTITY_ID, SP_ACS, null, requestAttributes); - // Verify the response includes our custom attributes - assertThat(samlResponse, containsString("department")); - assertThat(samlResponse, containsString("engineering")); - assertThat(samlResponse, containsString("product")); - assertThat(samlResponse, containsString("region")); - assertThat(samlResponse, containsString("APAC")); + // Parse XML directly from samlResponse (it's not base64 encoded at this point) + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); // Required for XPath + DocumentBuilder builder = factory.newDocumentBuilder(); + Document document = builder.parse(new InputSource(new StringReader(samlResponse))); + + // Create XPath evaluator + XPathFactory xPathFactory = XPathFactory.newInstance(); + XPath xpath = xPathFactory.newXPath(); + + // Validate SAML Response structure + Element responseElement = (Element) xpath.evaluate("//*[local-name()='Response']", document, XPathConstants.NODE); + assertThat("SAML Response element should exist", responseElement, notNullValue()); + + Element assertionElement = (Element) xpath.evaluate("//*[local-name()='Assertion']", document, XPathConstants.NODE); + assertThat("SAML Assertion element should exist", assertionElement, notNullValue()); + + // Validate department attribute + NodeList departmentAttributes = (NodeList) xpath.evaluate( + "//*[local-name()='Attribute' and @Name='department']/*[local-name()='AttributeValue']", + document, + XPathConstants.NODESET + ); + + assertThat("Should have two values for department attribute", departmentAttributes.getLength(), is(2)); + + // Verify department values + List departmentValues = new ArrayList<>(); + for (int i = 0; i < departmentAttributes.getLength(); i++) { + departmentValues.add(departmentAttributes.item(i).getTextContent()); + } + assertThat( + "Department attribute should contain 'engineering' and 'product'", + departmentValues, + containsInAnyOrder("engineering", "product") + ); + + // Validate region attribute + NodeList regionAttributes = (NodeList) xpath.evaluate( + "//*[local-name()='Attribute' and @Name='region']/*[local-name()='AttributeValue']", + document, + XPathConstants.NODESET + ); + + assertThat("Should have one value for region attribute", regionAttributes.getLength(), is(1)); + assertThat("Region attribute should contain 'APJ'", regionAttributes.item(0).getTextContent(), equalTo("APJ")); authenticateWithSamlResponse(samlResponse, null); } From 30a89c534be3834c50c060c8acf7ad5675507cfd Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Wed, 21 May 2025 17:28:53 +0900 Subject: [PATCH 08/15] Simplify SAML attributes representation using JSON object/Map structure Also, replace internal Attribute class list with a simpler Map> structure This change: - Removes the redundant Attribute class and replaces it with a direct Map implementation for storing attribute key-value pairs - Eliminates the duplicate "attributes" nesting in the JSON structure - Simplifies attribute validation without needing duplicate key checking - Updates all related tests and integration points to work with the new structure Before: ```js { // others "attributes": { "attributes": [ { "key": "department", "values": ["engineering", "product"] } ] } } After: ```js { // other "attributes": { "department": ["engineering", "product"] } } ``` (Verified by spitting out JSON entity in IdentityProviderAuthenticationIT.generateSamlResponseWithAttributes ... saw `{"entity_id":"ec:123456:abcdefg","acs":"https://sp1.test.es.elasticsearch.org/saml/acs","attributes":{"department":["engineering","product"],"region":["APJ"]}}`) Signed-off-by: lloydmeta --- .../idp/IdentityProviderAuthenticationIT.java | 12 +- ...lAuthenticationResponseMessageBuilder.java | 5 +- .../SamlInitiateSingleSignOnAttributes.java | 233 ++++-------- .../SamlInitiateSingleSignOnRequestTests.java | 30 +- ...enticationResponseMessageBuilderTests.java | 10 +- ...mlInitiateSingleSignOnAttributesTests.java | 343 +++++++----------- 6 files changed, 240 insertions(+), 393 deletions(-) diff --git a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java index 80f8c498f8632..7f3b39fb75d50 100644 --- a/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java +++ b/x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java @@ -109,16 +109,10 @@ public void testCustomAttributesInIdpInitiatedSso() throws Exception { ensureGreen(SamlServiceProviderIndex.INDEX_NAME); // Create custom attributes - List> attributesList = new ArrayList<>(); - Map attr1 = Map.of("key", "department", "values", List.of("engineering", "product")); - Map attr2 = Map.of("key", "region", "values", List.of("APJ")); - attributesList.add(attr1); - attributesList.add(attr2); - - final Map requestAttributes = Map.of("attributes", attributesList); + Map> attributesMap = Map.of("department", List.of("engineering", "product"), "region", List.of("APJ")); // Generate SAML response with custom attributes - final String samlResponse = generateSamlResponseWithAttributes(SP_ENTITY_ID, SP_ACS, null, requestAttributes); + final String samlResponse = generateSamlResponseWithAttributes(SP_ENTITY_ID, SP_ACS, null, attributesMap); // Parse XML directly from samlResponse (it's not base64 encoded at this point) DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); @@ -229,7 +223,7 @@ private String generateSamlResponseWithAttributes( String entityId, String acs, @Nullable Map authnState, - @Nullable Map attributes + @Nullable Map> attributes ) throws IOException { final Request request = new Request("POST", "/_idp/saml/init"); diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java index 2be14cff03b21..ad2375569d16a 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java @@ -45,6 +45,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Set; import static org.opensaml.saml.saml2.core.NameIDType.TRANSIENT; @@ -231,8 +232,8 @@ private AttributeStatement buildAttributes( } // Add custom attributes if provided if (customAttributes != null && customAttributes.getAttributes().isEmpty() == false) { - for (SamlInitiateSingleSignOnAttributes.Attribute customAttr : customAttributes.getAttributes()) { - Attribute attribute = buildAttribute(customAttr.getKey(), customAttr.getKey(), customAttr.getValues()); + for (Map.Entry> entry : customAttributes.getAttributes().entrySet()) { + Attribute attribute = buildAttribute(entry.getKey(), entry.getKey(), entry.getValue()); if (attribute != null) { attributes.add(attribute); } diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java index fa0d4b3de669a..f9caadbec2321 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java @@ -12,8 +12,6 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.xcontent.ObjectParser; -import org.elasticsearch.xcontent.ParseField; import org.elasticsearch.xcontent.ToXContentObject; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentParser; @@ -21,101 +19,111 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; -import java.util.Set; /** * Represents a collection of SAML attributes to be included in the SAML response. * Each attribute has a key and a list of values. */ public class SamlInitiateSingleSignOnAttributes implements Writeable, ToXContentObject { - private List attributes; + private Map> attributes; public SamlInitiateSingleSignOnAttributes() { - this.attributes = new ArrayList<>(); + this.attributes = new HashMap<>(); } - public SamlInitiateSingleSignOnAttributes(StreamInput in) throws IOException { - int size = in.readVInt(); - attributes = new ArrayList<>(size); - for (int i = 0; i < size; i++) { - attributes.add(new Attribute(in)); - } + /** + * @return A map of SAML attribute key to list of values + */ + public Map> getAttributes() { + return Collections.unmodifiableMap(attributes); + } + + public SamlInitiateSingleSignOnAttributes setAttributes(Map> attributes) { + this.attributes = new HashMap<>(attributes); + return this; } /** - * Validates this SAML attributes object to ensure all attribute keys are valid and unique. - * @return ActionRequestValidationException containing validation errors, or null if valid + * Creates a SamlInitiateSingleSignOnAttributes object by parsing the provided JSON content. + * Expects a JSON structure like: { "attr1": ["val1", "val2"], "attr2": ["val3"] } + * + * @param parser The XContentParser positioned at the start of the object + * @return A new SamlInitiateSingleSignOnAttributes instance */ - public ActionRequestValidationException validate() { - ActionRequestValidationException validationException = null; - - // Check for null/empty attribute keys and duplicate keys - if (attributes.isEmpty() == false) { - Set 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 - ); + public static SamlInitiateSingleSignOnAttributes fromXContent(XContentParser parser) throws IOException { + Map> attributes = new HashMap<>(); + while (parser.nextToken() != XContentParser.Token.END_OBJECT) { + String key = parser.currentName(); + if (parser.nextToken() == XContentParser.Token.START_ARRAY) { + List values = new ArrayList<>(); + while (parser.nextToken() != XContentParser.Token.END_ARRAY) { + values.add(parser.text()); } + attributes.put(key, values); } } - - return validationException; - } - - public List getAttributes() { - return Collections.unmodifiableList(attributes); - } - - public void setAttributes(List attributes) { - this.attributes = new ArrayList<>(attributes); - } - - public static final ObjectParser PARSER = new ObjectParser<>( - "saml_attributes", - true, - SamlInitiateSingleSignOnAttributes::new - ); - - static { - PARSER.declareObjectArray(SamlInitiateSingleSignOnAttributes::setAttributes, Attribute.PARSER, Fields.ATTRIBUTES); + SamlInitiateSingleSignOnAttributes attributesObj = new SamlInitiateSingleSignOnAttributes(); + attributesObj.setAttributes(attributes); + return attributesObj; } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(); - builder.startArray(Fields.ATTRIBUTES.getPreferredName()); - for (Attribute attribute : attributes) { - attribute.toXContent(builder, params); + for (Map.Entry> entry : attributes.entrySet()) { + builder.startArray(entry.getKey()); + for (String value : entry.getValue()) { + builder.value(value); + } + builder.endArray(); } - builder.endArray(); - return builder.endObject(); + return builder; } - public static SamlInitiateSingleSignOnAttributes fromXContent(XContentParser parser) throws IOException { - SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); - return PARSER.parse(parser, attributes, null); + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeVInt(attributes.size()); + for (Map.Entry> entry : attributes.entrySet()) { + out.writeString(entry.getKey()); + List values = entry.getValue(); + out.writeVInt(values.size()); + for (String value : values) { + out.writeString(value); + } + } } - public interface Fields { - ParseField ATTRIBUTES = new ParseField("attributes"); + public SamlInitiateSingleSignOnAttributes(StreamInput in) throws IOException { + int size = in.readVInt(); + attributes = new HashMap<>(size); + for (int i = 0; i < size; i++) { + String key = in.readString(); + int valuesSize = in.readVInt(); + List values = new ArrayList<>(valuesSize); + for (int j = 0; j < valuesSize; j++) { + values.add(in.readString()); + } + attributes.put(key, values); + } } - @Override - public void writeTo(StreamOutput out) throws IOException { - out.writeVInt(attributes.size()); - for (Attribute attribute : attributes) { - attribute.writeTo(out); + /** + * Validates the attributes for correctness. + * An attribute with an empty key is considered invalid. + */ + public ActionRequestValidationException validate() { + ActionRequestValidationException validationException = null; + if (attributes.isEmpty() == false) { + for (String key : attributes.keySet()) { + if (Strings.isNullOrEmpty(key)) { + validationException = ValidateActions.addValidationError("attribute key cannot be null or empty", validationException); + } + } } + return validationException; } @Override @@ -135,97 +143,4 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(attributes); } - - /** - * Individual attribute with a key and a list of values. - */ - public static class Attribute implements Writeable, ToXContentObject { - private String key; - private List values; - - public Attribute() { - this.values = new ArrayList<>(); - } - - public Attribute(String key, List values) { - this.key = key; - this.values = new ArrayList<>(values); - } - - public Attribute(StreamInput in) throws IOException { - this.key = in.readString(); - int size = in.readVInt(); - this.values = new ArrayList<>(size); - for (int i = 0; i < size; i++) { - values.add(in.readString()); - } - } - - public String getKey() { - return key; - } - - public void setKey(String key) { - this.key = key; - } - - public List getValues() { - return Collections.unmodifiableList(values); - } - - public void setValues(List values) { - this.values = new ArrayList<>(values); - } - - public static final ObjectParser PARSER = new ObjectParser<>("attribute", true, Attribute::new); - - static { - PARSER.declareString(Attribute::setKey, AttributeFields.KEY); - PARSER.declareStringArray(Attribute::setValues, AttributeFields.VALUES); - } - - @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(); - builder.field(AttributeFields.KEY.getPreferredName(), key); - builder.startArray(AttributeFields.VALUES.getPreferredName()); - for (String value : values) { - builder.value(value); - } - builder.endArray(); - return builder.endObject(); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - out.writeString(key); - out.writeVInt(values.size()); - for (String value : values) { - out.writeString(value); - } - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - Attribute attribute = (Attribute) o; - return Objects.equals(key, attribute.key) && Objects.equals(values, attribute.values); - } - - @Override - public int hashCode() { - return Objects.hash(key, values); - } - - @Override - public String toString() { - return getClass().getSimpleName() + Strings.toString(this); - } - - public interface AttributeFields { - ParseField KEY = new ParseField("key"); - ParseField VALUES = new ParseField("values"); - } - } } diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java index 85bd6aa0e1ca7..f48977b54a0ae 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java @@ -11,10 +11,11 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; @@ -52,31 +53,30 @@ public void testDuplicateAttributeKeysValidation() { request.setSpEntityId("https://kibana_url"); request.setAssertionConsumerService("https://kibana_url/acs"); - // Test with unique attribute keys - should be valid + // Test with valid attribute keys SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); - List attributeList = new ArrayList<>(); - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Collections.singletonList("value1"))); - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Arrays.asList("value2A", "value2B"))); - attributes.setAttributes(attributeList); + Map> attributeMap = new HashMap<>(); + attributeMap.put("key1", Collections.singletonList("value1")); + attributeMap.put("key2", Arrays.asList("value2A", "value2B")); + attributes.setAttributes(attributeMap); request.setAttributes(attributes); // Should pass validation ActionRequestValidationException validationException = request.validate(); - assertNull("Request with unique attribute keys should pass validation", validationException); + assertNull("Request with valid attribute keys should pass validation", validationException); - // Test with duplicate attribute keys - should be invalid + // Test with empty attribute key - should be invalid attributes = 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"))); - attributes.setAttributes(attributeList); + attributeMap = new HashMap<>(); + attributeMap.put("", Collections.singletonList("value1")); + attributeMap.put("unique_key", Collections.singletonList("value2")); + attributes.setAttributes(attributeMap); request.setAttributes(attributes); // Should fail validation with appropriate error message validationException = request.validate(); - assertNotNull("Request with duplicate attribute keys should fail validation", validationException); + assertNotNull("Request with empty attribute key should fail validation", validationException); assertThat(validationException.validationErrors().size(), equalTo(1)); - assertThat(validationException.validationErrors().get(0), containsString("duplicate attribute key [duplicate_key] found")); + assertThat(validationException.validationErrors().get(0), containsString("attribute key cannot be null or empty")); } } diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java index a91c344748d47..9312e0948e06c 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java @@ -25,7 +25,9 @@ import java.time.Duration; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Set; import static org.hamcrest.Matchers.containsString; @@ -61,15 +63,15 @@ public void testSignedResponseIsValidAgainstXmlSchema() throws Exception { public void testSignedResponseWithCustomAttributes() throws Exception { // Create custom attributes SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); - List attributeList = new ArrayList<>(); - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("customAttr1", Collections.singletonList("value1"))); + Map> attributeMap = new HashMap<>(); + attributeMap.put("customAttr1", Collections.singletonList("value1")); List multipleValues = new ArrayList<>(); multipleValues.add("value2A"); multipleValues.add("value2B"); - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("customAttr2", multipleValues)); + attributeMap.put("customAttr2", multipleValues); - attributes.setAttributes(attributeList); + attributes.setAttributes(attributeMap); // Build response with custom attributes final Response response = buildResponse(attributes); diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java index ba4bb2edc881c..fb9e078aebef2 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java @@ -4,123 +4,127 @@ * 2.0; you may not use this file except in compliance with the Elastic License * 2.0. */ + 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.common.bytes.BytesReference; +import org.elasticsearch.common.io.stream.InputStreamStreamInput; +import org.elasticsearch.common.io.stream.OutputStreamStreamOutput; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.ToXContent; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; import org.elasticsearch.xcontent.XContentParser; -import org.elasticsearch.xcontent.XContentType; +import org.elasticsearch.xcontent.json.JsonXContent; +import org.hamcrest.Matchers; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; -import java.util.ArrayList; +import java.io.InputStream; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasSize; -import static org.hamcrest.Matchers.not; public class SamlInitiateSingleSignOnAttributesTests extends ESTestCase { - public void testConstructors() { + public void testConstructors() throws Exception { // Test default constructor final SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(); - assertThat(attributes1.getAttributes(), hasSize(0)); + assertThat(attributes1.getAttributes(), Matchers.anEmptyMap()); - // Test with empty attribute list + // Test a second instance is also empty (not holding state) final SamlInitiateSingleSignOnAttributes attributes2 = new SamlInitiateSingleSignOnAttributes(); - attributes2.setAttributes(Collections.emptyList()); - assertThat(attributes2.getAttributes(), hasSize(0)); + assertThat(attributes2.getAttributes(), Matchers.anEmptyMap()); - // Test with non-empty attribute list - List attributeList = new ArrayList<>(); - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Collections.singletonList("value1"))); + // Test adding attributes + Map> attributeMap = new HashMap<>(); + attributeMap.put("key1", Collections.singletonList("value1")); final SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(); - attributes3.setAttributes(attributeList); - assertThat(attributes3.getAttributes(), hasSize(1)); - assertThat(attributes3.getAttributes().get(0).getKey(), equalTo("key1")); + attributes3.setAttributes(attributeMap); + assertThat(attributes3.getAttributes().size(), equalTo(1)); } - public void testEmptyAttributes() throws IOException { + public void testEmptyAttributes() throws Exception { final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); - assertThat(attributes.getAttributes(), hasSize(0)); - final XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON); + // Test toXContent + XContentBuilder builder = XContentFactory.jsonBuilder(); + builder.startObject(); attributes.toXContent(builder, ToXContent.EMPTY_PARAMS); - final String json = Strings.toString(builder); - assertThat(json, equalTo("{\"attributes\":[]}")); + builder.endObject(); + String json = BytesReference.bytes(builder).utf8ToString(); final SamlInitiateSingleSignOnAttributes parsedAttributes = parseFromJson(json); - assertThat(parsedAttributes.getAttributes(), hasSize(0)); + assertThat(parsedAttributes.getAttributes(), Matchers.anEmptyMap()); - // Test wire serialization + // Test serialization SamlInitiateSingleSignOnAttributes serialized = copySerialize(attributes); - assertThat(serialized.getAttributes(), hasSize(0)); + assertThat(serialized.getAttributes(), Matchers.anEmptyMap()); } - public void testWithAttributes() throws IOException { + public void testWithAttributes() throws Exception { final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); - List attributeList = new ArrayList<>(); - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "value2"))); - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Collections.singletonList("value3"))); - - attributes.setAttributes(attributeList); - - assertThat(attributes.getAttributes(), hasSize(2)); - assertThat(attributes.getAttributes().get(0).getKey(), equalTo("key1")); - assertThat(attributes.getAttributes().get(0).getValues(), hasSize(2)); - assertThat(attributes.getAttributes().get(0).getValues().get(0), equalTo("value1")); - assertThat(attributes.getAttributes().get(0).getValues().get(1), equalTo("value2")); - - assertThat(attributes.getAttributes().get(1).getKey(), equalTo("key2")); - assertThat(attributes.getAttributes().get(1).getValues(), hasSize(1)); - assertThat(attributes.getAttributes().get(1).getValues().get(0), equalTo("value3")); - - final XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON); + Map> attributeMap = new HashMap<>(); + attributeMap.put("key1", Arrays.asList("value1", "value2")); + attributeMap.put("key2", Collections.singletonList("value3")); + attributes.setAttributes(attributeMap); + + // Test getAttributes + Map> returnedAttributes = attributes.getAttributes(); + assertThat(returnedAttributes.size(), equalTo(2)); + assertThat(returnedAttributes.get("key1").size(), equalTo(2)); + assertThat(returnedAttributes.get("key1").get(0), equalTo("value1")); + assertThat(returnedAttributes.get("key1").get(1), equalTo("value2")); + assertThat(returnedAttributes.get("key2").size(), equalTo(1)); + assertThat(returnedAttributes.get("key2").get(0), equalTo("value3")); + + // Test immutability of returned attributes + expectThrows(UnsupportedOperationException.class, () -> returnedAttributes.put("newKey", Collections.singletonList("value"))); + expectThrows(UnsupportedOperationException.class, () -> returnedAttributes.get("key1").add("value3")); + + // Test validate + ActionRequestValidationException validationException = attributes.validate(); + assertNull(validationException); + + // Test toXContent + XContentBuilder builder = XContentFactory.jsonBuilder(); + builder.startObject(); attributes.toXContent(builder, ToXContent.EMPTY_PARAMS); - final String json = Strings.toString(builder); - - // Check that JSON contains expected values - assertThat(json, containsString("key1")); - assertThat(json, containsString("value1")); - assertThat(json, containsString("value2")); - assertThat(json, containsString("key2")); - assertThat(json, containsString("value3")); + builder.endObject(); + String json = BytesReference.bytes(builder).utf8ToString(); - // Check that parsed JSON matches original attributes + // Test parsing from JSON final SamlInitiateSingleSignOnAttributes parsedAttributes = parseFromJson(json); - assertThat(parsedAttributes.getAttributes(), hasSize(2)); - assertThat(parsedAttributes.getAttributes().get(0).getKey(), equalTo("key1")); - assertThat(parsedAttributes.getAttributes().get(0).getValues(), hasSize(2)); - assertThat(parsedAttributes.getAttributes().get(1).getKey(), equalTo("key2")); - assertThat(parsedAttributes.getAttributes().get(1).getValues(), hasSize(1)); - - // Test equals/hashCode - assertThat(attributes, equalTo(parsedAttributes)); - assertThat(attributes.hashCode(), equalTo(parsedAttributes.hashCode())); - - // Test wire serialization + assertThat(parsedAttributes.getAttributes().size(), equalTo(2)); + assertThat(parsedAttributes.getAttributes().get("key1").size(), equalTo(2)); + assertThat(parsedAttributes.getAttributes().get("key1").get(0), equalTo("value1")); + assertThat(parsedAttributes.getAttributes().get("key1").get(1), equalTo("value2")); + assertThat(parsedAttributes.getAttributes().get("key2").size(), equalTo(1)); + assertThat(parsedAttributes.getAttributes().get("key2").get(0), equalTo("value3")); + + // Test serialization SamlInitiateSingleSignOnAttributes serialized = copySerialize(attributes); - assertThat(serialized.getAttributes(), hasSize(2)); - assertThat(serialized.getAttributes().get(0).getKey(), equalTo("key1")); - assertThat(serialized.getAttributes().get(0).getValues(), hasSize(2)); - assertThat(serialized.getAttributes().get(1).getKey(), equalTo("key2")); - assertThat(serialized.getAttributes().get(1).getValues(), hasSize(1)); + assertThat(serialized.getAttributes().size(), equalTo(2)); + assertThat(serialized.getAttributes().get("key1").size(), equalTo(2)); + assertThat(serialized.getAttributes().get("key1").get(0), equalTo("value1")); + assertThat(serialized.getAttributes().get("key1").get(1), equalTo("value2")); + assertThat(serialized.getAttributes().get("key2").size(), equalTo(1)); + assertThat(serialized.getAttributes().get("key2").get(0), equalTo("value3")); } public void testToString() { final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); - List attributeList = new ArrayList<>(); - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "value2"))); - attributes.setAttributes(attributeList); + Map> attributeMap = new HashMap<>(); + attributeMap.put("key1", Arrays.asList("value1", "value2")); + attributes.setAttributes(attributeMap); String toString = attributes.toString(); assertThat(toString, containsString("SamlInitiateSingleSignOnAttributes")); @@ -128,171 +132,102 @@ public void testToString() { assertThat(toString, containsString("value1")); assertThat(toString, containsString("value2")); - // Test with multiple attributes - attributeList.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Collections.singletonList("value3"))); - attributes.setAttributes(attributeList); + // Add another attribute + attributeMap.put("key2", Collections.singletonList("value3")); + attributes.setAttributes(attributeMap); + toString = attributes.toString(); - assertThat(toString, containsString("key1")); assertThat(toString, containsString("key2")); assertThat(toString, containsString("value3")); - // Test with empty attributes + // Test empty attributes final SamlInitiateSingleSignOnAttributes emptyAttributes = new SamlInitiateSingleSignOnAttributes(); toString = emptyAttributes.toString(); assertThat(toString, containsString("SamlInitiateSingleSignOnAttributes")); - assertThat(toString, containsString("attributes=[]")); + assertThat(toString, containsString("attributes={}")); } - public void testAttributeClass() throws IOException { - SamlInitiateSingleSignOnAttributes.Attribute attribute = new SamlInitiateSingleSignOnAttributes.Attribute( - "testKey", - Arrays.asList("val1", "val2") - ); - - assertThat(attribute.getKey(), equalTo("testKey")); - assertThat(attribute.getValues(), hasSize(2)); - assertThat(attribute.getValues().get(0), equalTo("val1")); - assertThat(attribute.getValues().get(1), equalTo("val2")); - - // Test equals/hashCode - SamlInitiateSingleSignOnAttributes.Attribute attribute2 = new SamlInitiateSingleSignOnAttributes.Attribute( - "testKey", - Arrays.asList("val1", "val2") - ); - assertThat(attribute, equalTo(attribute2)); - assertThat(attribute.hashCode(), equalTo(attribute2.hashCode())); - - SamlInitiateSingleSignOnAttributes.Attribute attribute3 = new SamlInitiateSingleSignOnAttributes.Attribute( - "differentKey", - Arrays.asList("val1", "val2") - ); - assertThat(attribute, not(equalTo(attribute3))); - - SamlInitiateSingleSignOnAttributes.Attribute attribute4 = new SamlInitiateSingleSignOnAttributes.Attribute( - "testKey", - Arrays.asList("differentValue") - ); - assertThat(attribute, not(equalTo(attribute4))); - - // Test equals with different object types - assertThat(attribute.equals("someString"), equalTo(false)); - assertThat(attribute.equals(null), equalTo(false)); - - // Test setter methods - SamlInitiateSingleSignOnAttributes.Attribute mutableAttr = new SamlInitiateSingleSignOnAttributes.Attribute( - "testKey", - Collections.emptyList() - ); - mutableAttr.setKey("newKey"); - mutableAttr.setValues(Arrays.asList("newVal1", "newVal2")); - assertThat(mutableAttr.getKey(), equalTo("newKey")); - assertThat(mutableAttr.getValues(), hasSize(2)); - assertThat(mutableAttr.getValues().get(0), equalTo("newVal1")); - assertThat(mutableAttr.getValues().get(1), equalTo("newVal2")); - - // Test toString - String toString = attribute.toString(); - assertThat(toString, containsString("Attribute")); - assertThat(toString, containsString("testKey")); - assertThat(toString, containsString("val1")); + public void testValidation() throws Exception { + // Test validation with empty key + SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); + Map> attributeMap = new HashMap<>(); + attributeMap.put("", Arrays.asList("value1", "value2")); + attributes.setAttributes(attributeMap); + + ActionRequestValidationException validationException = attributes.validate(); + assertNotNull(validationException); + assertThat(validationException.getMessage(), containsString("attribute key cannot be null or empty")); + + // Test validation with null key + attributeMap = new HashMap<>(); + attributeMap.put(null, Collections.singletonList("value")); + attributes.setAttributes(attributeMap); + + validationException = attributes.validate(); + assertNotNull(validationException); + assertThat(validationException.getMessage(), containsString("attribute key cannot be null or empty")); } public void testEqualsAndHashCode() { - // Create attributes objects with the same content - List attributeList1 = new ArrayList<>(); - attributeList1.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "value2"))); - attributeList1.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Collections.singletonList("value3"))); + Map> attributeMap1 = new HashMap<>(); + attributeMap1.put("key1", Arrays.asList("value1", "value2")); + attributeMap1.put("key2", Collections.singletonList("value3")); SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(); - attributes1.setAttributes(attributeList1); + attributes1.setAttributes(attributeMap1); - List attributeList2 = new ArrayList<>(); - attributeList2.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "value2"))); - attributeList2.add(new SamlInitiateSingleSignOnAttributes.Attribute("key2", Collections.singletonList("value3"))); + Map> attributeMap2 = new HashMap<>(); + attributeMap2.put("key1", Arrays.asList("value1", "value2")); + attributeMap2.put("key2", Collections.singletonList("value3")); SamlInitiateSingleSignOnAttributes attributes2 = new SamlInitiateSingleSignOnAttributes(); - attributes2.setAttributes(attributeList2); + attributes2.setAttributes(attributeMap2); - // Test equals and hashCode for identical objects - assertThat(attributes1, equalTo(attributes2)); + // Test equals + assertTrue(attributes1.equals(attributes2)); + assertTrue(attributes2.equals(attributes1)); + + // Test hashCode assertThat(attributes1.hashCode(), equalTo(attributes2.hashCode())); - // Test equals with self - assertThat(attributes1.equals(attributes1), equalTo(true)); + // Test with different values + Map> attributeMap3 = new HashMap<>(); + attributeMap3.put("key1", Arrays.asList("different", "value2")); + attributeMap3.put("key2", Collections.singletonList("value3")); - // Test different objects - List attributeList3 = new ArrayList<>(); - attributeList3.add(new SamlInitiateSingleSignOnAttributes.Attribute("key1", Arrays.asList("value1", "different"))); SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(); - attributes3.setAttributes(attributeList3); + attributes3.setAttributes(attributeMap3); - assertThat(attributes1.equals(attributes3), equalTo(false)); + assertFalse(attributes1.equals(attributes3)); - // Test equals with null and different object types - assertThat(attributes1.equals(null), equalTo(false)); - assertThat(attributes1.equals("string"), equalTo(false)); - } + // Test with missing key + Map> attributeMap4 = new HashMap<>(); + attributeMap4.put("key1", Arrays.asList("value1", "value2")); - public void testValidate() { - // Test with valid attributes - should pass validation - final SamlInitiateSingleSignOnAttributes validAttributes = new SamlInitiateSingleSignOnAttributes(); - List 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")); + SamlInitiateSingleSignOnAttributes attributes4 = new SamlInitiateSingleSignOnAttributes(); + attributes4.setAttributes(attributeMap4); + + assertFalse(attributes1.equals(attributes4)); } private SamlInitiateSingleSignOnAttributes parseFromJson(String json) throws IOException { - try (XContentParser parser = createParser(XContentType.JSON.xContent(), json)) { + try ( + InputStream stream = new ByteArrayInputStream(json.getBytes("UTF-8")); + XContentParser parser = JsonXContent.jsonXContent.createParser(null, null, stream) + ) { parser.nextToken(); // Start object return SamlInitiateSingleSignOnAttributes.fromXContent(parser); } } - private SamlInitiateSingleSignOnAttributes copySerialize(SamlInitiateSingleSignOnAttributes attributes) throws IOException { - try (BytesStreamOutput out = new BytesStreamOutput()) { - attributes.writeTo(out); - try (var in = out.bytes().streamInput()) { - return new SamlInitiateSingleSignOnAttributes(in); - } - } + private SamlInitiateSingleSignOnAttributes copySerialize(SamlInitiateSingleSignOnAttributes original) throws IOException { + ByteArrayOutputStream outBuffer = new ByteArrayOutputStream(); + OutputStreamStreamOutput out = new OutputStreamStreamOutput(outBuffer); + original.writeTo(out); + out.flush(); + + ByteArrayInputStream inBuffer = new ByteArrayInputStream(outBuffer.toByteArray()); + InputStreamStreamInput in = new InputStreamStreamInput(inBuffer); + return new SamlInitiateSingleSignOnAttributes(in); } } From 2d10dc1d11393fa1803cb8460b734d0827ffe330 Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Thu, 22 May 2025 17:20:28 +0900 Subject: [PATCH 09/15] * Fix up toString dangling quote. Signed-off-by: lloydmeta --- .../xpack/idp/action/SamlInitiateSingleSignOnRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java index 882da7c5632ed..d3bc570125725 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java @@ -109,7 +109,7 @@ public String toString() { + "acs='" + assertionConsumerService + "', " - + "attributes=" + + "attributes='" + attributes + "'}"; } From be900e64e76a00e231be3d8fb20883968c04c2ae Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Mon, 2 Jun 2025 10:51:10 +0900 Subject: [PATCH 10/15] * Remove attributes from Response object. Signed-off-by: lloydmeta --- .../SamlInitiateSingleSignOnResponse.java | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnResponse.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnResponse.java index ba3ca1df9d177..c0a5157557f58 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnResponse.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnResponse.java @@ -10,7 +10,6 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.core.Nullable; import org.elasticsearch.xcontent.XContentBuilder; -import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; import java.io.IOException; @@ -21,7 +20,6 @@ public class SamlInitiateSingleSignOnResponse extends ActionResponse { private final String entityId; private final String samlStatus; private final String error; - private final SamlInitiateSingleSignOnAttributes attributes; public SamlInitiateSingleSignOnResponse( String entityId, @@ -29,24 +27,12 @@ public SamlInitiateSingleSignOnResponse( String samlResponse, String samlStatus, @Nullable String error - ) { - this(entityId, postUrl, samlResponse, samlStatus, error, null); - } - - public SamlInitiateSingleSignOnResponse( - String entityId, - String postUrl, - String samlResponse, - String samlStatus, - @Nullable String error, - @Nullable SamlInitiateSingleSignOnAttributes attributes ) { this.entityId = entityId; this.postUrl = postUrl; this.samlResponse = samlResponse; this.samlStatus = samlStatus; this.error = error; - this.attributes = attributes; } public String getPostUrl() { @@ -69,10 +55,6 @@ public String getSamlStatus() { return samlStatus; } - public SamlInitiateSingleSignOnAttributes getAttributes() { - return attributes; - } - @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(entityId); @@ -80,7 +62,6 @@ public void writeTo(StreamOutput out) throws IOException { out.writeString(samlResponse); out.writeString(samlStatus); out.writeOptionalString(error); - out.writeOptionalWriteable(attributes); } public void toXContent(XContentBuilder builder) throws IOException { From 04e8c4594e185fd07ccd7fa2009fa78fdb4a6b2b Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Mon, 2 Jun 2025 11:22:05 +0900 Subject: [PATCH 11/15] * Remove friendly name. * Make attributes map final in SamlInitiateSingleSignOnAttributes Signed-off-by: lloydmeta --- ...lAuthenticationResponseMessageBuilder.java | 10 ++-- .../SamlInitiateSingleSignOnAttributes.java | 15 ++---- .../SamlInitiateSingleSignOnRequestTests.java | 6 +-- ...enticationResponseMessageBuilderTests.java | 4 +- ...mlInitiateSingleSignOnAttributesTests.java | 54 ++++++------------- 5 files changed, 29 insertions(+), 60 deletions(-) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java index ad2375569d16a..301ada3561299 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java @@ -233,7 +233,7 @@ private AttributeStatement buildAttributes( // Add custom attributes if provided if (customAttributes != null && customAttributes.getAttributes().isEmpty() == false) { for (Map.Entry> entry : customAttributes.getAttributes().entrySet()) { - Attribute attribute = buildAttribute(entry.getKey(), entry.getKey(), entry.getValue()); + Attribute attribute = buildAttribute(entry.getKey(), null, entry.getValue()); if (attribute != null) { attributes.add(attribute); } @@ -247,20 +247,22 @@ private AttributeStatement buildAttributes( return statement; } - private Attribute buildAttribute(String formalName, String friendlyName, String value) { + private Attribute buildAttribute(String formalName, @Nullable String friendlyName, String value) { if (Strings.isNullOrEmpty(value)) { return null; } return buildAttribute(formalName, friendlyName, List.of(value)); } - private Attribute buildAttribute(String formalName, String friendlyName, Collection values) { + private Attribute buildAttribute(String formalName, @Nullable String friendlyName, Collection values) { if (values.isEmpty() || Strings.isNullOrEmpty(formalName)) { return null; } final Attribute attribute = samlFactory.object(Attribute.class, Attribute.DEFAULT_ELEMENT_NAME); attribute.setName(formalName); - attribute.setFriendlyName(friendlyName); + if (Strings.isNullOrEmpty(friendlyName) == false) { + attribute.setFriendlyName(friendlyName); + } attribute.setNameFormat(Attribute.URI_REFERENCE); for (String val : values) { final XSString string = samlFactory.object(XSString.class, AttributeValue.DEFAULT_ELEMENT_NAME, XSString.TYPE_NAME); diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java index f9caadbec2321..56477c92558fb 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java @@ -29,10 +29,10 @@ * Each attribute has a key and a list of values. */ public class SamlInitiateSingleSignOnAttributes implements Writeable, ToXContentObject { - private Map> attributes; + private final Map> attributes; - public SamlInitiateSingleSignOnAttributes() { - this.attributes = new HashMap<>(); + public SamlInitiateSingleSignOnAttributes(Map> attributes) { + this.attributes = attributes; } /** @@ -42,11 +42,6 @@ public Map> getAttributes() { return Collections.unmodifiableMap(attributes); } - public SamlInitiateSingleSignOnAttributes setAttributes(Map> attributes) { - this.attributes = new HashMap<>(attributes); - return this; - } - /** * Creates a SamlInitiateSingleSignOnAttributes object by parsing the provided JSON content. * Expects a JSON structure like: { "attr1": ["val1", "val2"], "attr2": ["val3"] } @@ -66,9 +61,7 @@ public static SamlInitiateSingleSignOnAttributes fromXContent(XContentParser par attributes.put(key, values); } } - SamlInitiateSingleSignOnAttributes attributesObj = new SamlInitiateSingleSignOnAttributes(); - attributesObj.setAttributes(attributes); - return attributesObj; + return new SamlInitiateSingleSignOnAttributes(attributes); } @Override diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java index f48977b54a0ae..afd07ab0fc36a 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java @@ -54,11 +54,10 @@ public void testDuplicateAttributeKeysValidation() { request.setAssertionConsumerService("https://kibana_url/acs"); // Test with valid attribute keys - SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); Map> attributeMap = new HashMap<>(); attributeMap.put("key1", Collections.singletonList("value1")); attributeMap.put("key2", Arrays.asList("value2A", "value2B")); - attributes.setAttributes(attributeMap); + SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(attributeMap); request.setAttributes(attributes); // Should pass validation @@ -66,11 +65,10 @@ public void testDuplicateAttributeKeysValidation() { assertNull("Request with valid attribute keys should pass validation", validationException); // Test with empty attribute key - should be invalid - attributes = new SamlInitiateSingleSignOnAttributes(); attributeMap = new HashMap<>(); attributeMap.put("", Collections.singletonList("value1")); attributeMap.put("unique_key", Collections.singletonList("value2")); - attributes.setAttributes(attributeMap); + attributes = new SamlInitiateSingleSignOnAttributes(attributeMap); request.setAttributes(attributes); // Should fail validation with appropriate error message diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java index 9312e0948e06c..0e58935c07e31 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java @@ -62,7 +62,6 @@ public void testSignedResponseIsValidAgainstXmlSchema() throws Exception { public void testSignedResponseWithCustomAttributes() throws Exception { // Create custom attributes - SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); Map> attributeMap = new HashMap<>(); attributeMap.put("customAttr1", Collections.singletonList("value1")); @@ -70,8 +69,7 @@ public void testSignedResponseWithCustomAttributes() throws Exception { multipleValues.add("value2A"); multipleValues.add("value2B"); attributeMap.put("customAttr2", multipleValues); - - attributes.setAttributes(attributeMap); + SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(attributeMap); // Build response with custom attributes final Response response = buildResponse(attributes); diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java index fb9e078aebef2..e25add68c9c24 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java @@ -35,24 +35,18 @@ public class SamlInitiateSingleSignOnAttributesTests extends ESTestCase { public void testConstructors() throws Exception { - // Test default constructor - final SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(); + final SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(Collections.emptyMap()); assertThat(attributes1.getAttributes(), Matchers.anEmptyMap()); - // Test a second instance is also empty (not holding state) - final SamlInitiateSingleSignOnAttributes attributes2 = new SamlInitiateSingleSignOnAttributes(); - assertThat(attributes2.getAttributes(), Matchers.anEmptyMap()); - // Test adding attributes Map> attributeMap = new HashMap<>(); attributeMap.put("key1", Collections.singletonList("value1")); - final SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(); - attributes3.setAttributes(attributeMap); + final SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(attributeMap); assertThat(attributes3.getAttributes().size(), equalTo(1)); } public void testEmptyAttributes() throws Exception { - final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); + final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(Collections.emptyMap()); // Test toXContent XContentBuilder builder = XContentFactory.jsonBuilder(); @@ -70,12 +64,10 @@ public void testEmptyAttributes() throws Exception { } public void testWithAttributes() throws Exception { - final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); - Map> attributeMap = new HashMap<>(); attributeMap.put("key1", Arrays.asList("value1", "value2")); attributeMap.put("key2", Collections.singletonList("value3")); - attributes.setAttributes(attributeMap); + final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(attributeMap); // Test getAttributes Map> returnedAttributes = attributes.getAttributes(); @@ -121,10 +113,9 @@ public void testWithAttributes() throws Exception { } public void testToString() { - final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); Map> attributeMap = new HashMap<>(); attributeMap.put("key1", Arrays.asList("value1", "value2")); - attributes.setAttributes(attributeMap); + final SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(attributeMap); String toString = attributes.toString(); assertThat(toString, containsString("SamlInitiateSingleSignOnAttributes")); @@ -132,16 +123,8 @@ public void testToString() { assertThat(toString, containsString("value1")); assertThat(toString, containsString("value2")); - // Add another attribute - attributeMap.put("key2", Collections.singletonList("value3")); - attributes.setAttributes(attributeMap); - - toString = attributes.toString(); - assertThat(toString, containsString("key2")); - assertThat(toString, containsString("value3")); - // Test empty attributes - final SamlInitiateSingleSignOnAttributes emptyAttributes = new SamlInitiateSingleSignOnAttributes(); + final SamlInitiateSingleSignOnAttributes emptyAttributes = new SamlInitiateSingleSignOnAttributes(Collections.emptyMap()); toString = emptyAttributes.toString(); assertThat(toString, containsString("SamlInitiateSingleSignOnAttributes")); assertThat(toString, containsString("attributes={}")); @@ -149,10 +132,9 @@ public void testToString() { public void testValidation() throws Exception { // Test validation with empty key - SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(); Map> attributeMap = new HashMap<>(); attributeMap.put("", Arrays.asList("value1", "value2")); - attributes.setAttributes(attributeMap); + SamlInitiateSingleSignOnAttributes attributes = new SamlInitiateSingleSignOnAttributes(attributeMap); ActionRequestValidationException validationException = attributes.validate(); assertNotNull(validationException); @@ -161,7 +143,7 @@ public void testValidation() throws Exception { // Test validation with null key attributeMap = new HashMap<>(); attributeMap.put(null, Collections.singletonList("value")); - attributes.setAttributes(attributeMap); + attributes = new SamlInitiateSingleSignOnAttributes(attributeMap); validationException = attributes.validate(); assertNotNull(validationException); @@ -173,19 +155,17 @@ public void testEqualsAndHashCode() { attributeMap1.put("key1", Arrays.asList("value1", "value2")); attributeMap1.put("key2", Collections.singletonList("value3")); - SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(); - attributes1.setAttributes(attributeMap1); + SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(attributeMap1); Map> attributeMap2 = new HashMap<>(); attributeMap2.put("key1", Arrays.asList("value1", "value2")); attributeMap2.put("key2", Collections.singletonList("value3")); - SamlInitiateSingleSignOnAttributes attributes2 = new SamlInitiateSingleSignOnAttributes(); - attributes2.setAttributes(attributeMap2); + SamlInitiateSingleSignOnAttributes attributes2 = new SamlInitiateSingleSignOnAttributes(attributeMap2); // Test equals - assertTrue(attributes1.equals(attributes2)); - assertTrue(attributes2.equals(attributes1)); + assertEquals(attributes1, attributes2); + assertEquals(attributes2, attributes1); // Test hashCode assertThat(attributes1.hashCode(), equalTo(attributes2.hashCode())); @@ -195,19 +175,17 @@ public void testEqualsAndHashCode() { attributeMap3.put("key1", Arrays.asList("different", "value2")); attributeMap3.put("key2", Collections.singletonList("value3")); - SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(); - attributes3.setAttributes(attributeMap3); + SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(attributeMap3); - assertFalse(attributes1.equals(attributes3)); + assertNotEquals(attributes1, attributes3); // Test with missing key Map> attributeMap4 = new HashMap<>(); attributeMap4.put("key1", Arrays.asList("value1", "value2")); - SamlInitiateSingleSignOnAttributes attributes4 = new SamlInitiateSingleSignOnAttributes(); - attributes4.setAttributes(attributeMap4); + SamlInitiateSingleSignOnAttributes attributes4 = new SamlInitiateSingleSignOnAttributes(attributeMap4); - assertFalse(attributes1.equals(attributes4)); + assertNotEquals(attributes1, attributes4); } private SamlInitiateSingleSignOnAttributes parseFromJson(String json) throws IOException { From 686544fe9f721e7b6b60f59746816ffbd00eb6bb Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Mon, 2 Jun 2025 11:41:56 +0900 Subject: [PATCH 12/15] * Cleanup serdes by using existing utils in the ES codebase Signed-off-by: lloydmeta --- .../SamlInitiateSingleSignOnAttributes.java | 45 +++---------------- ...mlInitiateSingleSignOnAttributesTests.java | 4 -- 2 files changed, 5 insertions(+), 44 deletions(-) diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java index 56477c92558fb..e6d533a7bc224 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributes.java @@ -12,12 +12,12 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.XContentParserUtils; import org.elasticsearch.xcontent.ToXContentObject; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentParser; import java.io.IOException; -import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -50,57 +50,22 @@ public Map> getAttributes() { * @return A new SamlInitiateSingleSignOnAttributes instance */ public static SamlInitiateSingleSignOnAttributes fromXContent(XContentParser parser) throws IOException { - Map> attributes = new HashMap<>(); - while (parser.nextToken() != XContentParser.Token.END_OBJECT) { - String key = parser.currentName(); - if (parser.nextToken() == XContentParser.Token.START_ARRAY) { - List values = new ArrayList<>(); - while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - values.add(parser.text()); - } - attributes.put(key, values); - } - } + final Map> attributes = parser.map(HashMap::new, p -> XContentParserUtils.parseList(p, XContentParser::text)); return new SamlInitiateSingleSignOnAttributes(attributes); } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - for (Map.Entry> entry : attributes.entrySet()) { - builder.startArray(entry.getKey()); - for (String value : entry.getValue()) { - builder.value(value); - } - builder.endArray(); - } - return builder; + return builder.map(attributes); } @Override public void writeTo(StreamOutput out) throws IOException { - out.writeVInt(attributes.size()); - for (Map.Entry> entry : attributes.entrySet()) { - out.writeString(entry.getKey()); - List values = entry.getValue(); - out.writeVInt(values.size()); - for (String value : values) { - out.writeString(value); - } - } + out.writeMap(attributes, StreamOutput::writeStringCollection); } public SamlInitiateSingleSignOnAttributes(StreamInput in) throws IOException { - int size = in.readVInt(); - attributes = new HashMap<>(size); - for (int i = 0; i < size; i++) { - String key = in.readString(); - int valuesSize = in.readVInt(); - List values = new ArrayList<>(valuesSize); - for (int j = 0; j < valuesSize; j++) { - values.add(in.readString()); - } - attributes.put(key, values); - } + this.attributes = in.readImmutableMap(StreamInput::readStringCollectionAsImmutableList); } /** diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java index e25add68c9c24..7a9fb5442705a 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java @@ -50,9 +50,7 @@ public void testEmptyAttributes() throws Exception { // Test toXContent XContentBuilder builder = XContentFactory.jsonBuilder(); - builder.startObject(); attributes.toXContent(builder, ToXContent.EMPTY_PARAMS); - builder.endObject(); String json = BytesReference.bytes(builder).utf8ToString(); final SamlInitiateSingleSignOnAttributes parsedAttributes = parseFromJson(json); @@ -88,9 +86,7 @@ public void testWithAttributes() throws Exception { // Test toXContent XContentBuilder builder = XContentFactory.jsonBuilder(); - builder.startObject(); attributes.toXContent(builder, ToXContent.EMPTY_PARAMS); - builder.endObject(); String json = BytesReference.bytes(builder).utf8ToString(); // Test parsing from JSON From fbb5a6f9713c5f315425230adfc90d27ca20d7e9 Mon Sep 17 00:00:00 2001 From: lloydmeta Date: Mon, 2 Jun 2025 11:45:00 +0900 Subject: [PATCH 13/15] Touchup comment Signed-off-by: lloydmeta --- .../saml/support/SamlInitiateSingleSignOnAttributesTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java index 7a9fb5442705a..5bf38ad8e8260 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/support/SamlInitiateSingleSignOnAttributesTests.java @@ -38,7 +38,7 @@ public void testConstructors() throws Exception { final SamlInitiateSingleSignOnAttributes attributes1 = new SamlInitiateSingleSignOnAttributes(Collections.emptyMap()); assertThat(attributes1.getAttributes(), Matchers.anEmptyMap()); - // Test adding attributes + // Test with non-empty map Map> attributeMap = new HashMap<>(); attributeMap.put("key1", Collections.singletonList("value1")); final SamlInitiateSingleSignOnAttributes attributes3 = new SamlInitiateSingleSignOnAttributes(attributeMap); From a28adf3e1a2872b2466ca57ac749fece88f64de9 Mon Sep 17 00:00:00 2001 From: Lloyd Date: Mon, 2 Jun 2025 17:39:15 +0900 Subject: [PATCH 14/15] Update x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java Co-authored-by: Tim Vernum --- .../xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java index afd07ab0fc36a..5a7e48551f877 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java @@ -47,7 +47,7 @@ public void testValidation() { assertThat(validationException.validationErrors().get(1), containsString("acs is missing")); } - public void testDuplicateAttributeKeysValidation() { + public void testBlankAttributeKeysValidation() { // Create request with valid required fields final SamlInitiateSingleSignOnRequest request = new SamlInitiateSingleSignOnRequest(); request.setSpEntityId("https://kibana_url"); From 5072c44ad324ddba4595f208c6cd6bb81bf65d2b Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Tue, 3 Jun 2025 11:22:06 +1000 Subject: [PATCH 15/15] Add transport-version checks --- .../org/elasticsearch/TransportVersions.java | 1 + .../SamlInitiateSingleSignOnRequest.java | 9 ++- .../SamlInitiateSingleSignOnRequestTests.java | 73 +++++++++++++++++-- 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/TransportVersions.java b/server/src/main/java/org/elasticsearch/TransportVersions.java index 57ba6de0b973c..440aa8cacc903 100644 --- a/server/src/main/java/org/elasticsearch/TransportVersions.java +++ b/server/src/main/java/org/elasticsearch/TransportVersions.java @@ -273,6 +273,7 @@ static TransportVersion def(int id) { public static final TransportVersion INFERENCE_CUSTOM_SERVICE_ADDED = def(9_084_0_00); public static final TransportVersion ESQL_LIMIT_ROW_SIZE = def(9_085_0_00); public static final TransportVersion ESQL_REGEX_MATCH_WITH_CASE_INSENSITIVITY = def(9_086_0_00); + public static final TransportVersion IDP_CUSTOM_SAML_ATTRIBUTES = def(9_087_0_00); /* * STOP! READ THIS FIRST! No, really, diff --git a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java index d3bc570125725..b93616f54fb3a 100644 --- a/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java +++ b/x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequest.java @@ -6,6 +6,7 @@ */ package org.elasticsearch.xpack.idp.action; +import org.elasticsearch.TransportVersions; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.LegacyActionRequest; import org.elasticsearch.common.Strings; @@ -30,7 +31,9 @@ public SamlInitiateSingleSignOnRequest(StreamInput in) throws IOException { spEntityId = in.readString(); assertionConsumerService = in.readString(); samlAuthenticationState = in.readOptionalWriteable(SamlAuthenticationState::new); - attributes = in.readOptionalWriteable(SamlInitiateSingleSignOnAttributes::new); + if (in.getTransportVersion().onOrAfter(TransportVersions.IDP_CUSTOM_SAML_ATTRIBUTES)) { + attributes = in.readOptionalWriteable(SamlInitiateSingleSignOnAttributes::new); + } } public SamlInitiateSingleSignOnRequest() {} @@ -96,7 +99,9 @@ public void writeTo(StreamOutput out) throws IOException { out.writeString(spEntityId); out.writeString(assertionConsumerService); out.writeOptionalWriteable(samlAuthenticationState); - out.writeOptionalWriteable(attributes); + if (out.getTransportVersion().onOrAfter(TransportVersions.IDP_CUSTOM_SAML_ATTRIBUTES)) { + out.writeOptionalWriteable(attributes); + } } @Override diff --git a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java index 5a7e48551f877..4a0ec674ab4c8 100644 --- a/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java +++ b/x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/SamlInitiateSingleSignOnRequestTests.java @@ -6,9 +6,13 @@ */ package org.elasticsearch.xpack.idp.action; +import org.elasticsearch.TransportVersion; +import org.elasticsearch.TransportVersions; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.TransportVersionUtils; import org.elasticsearch.xpack.idp.saml.support.SamlInitiateSingleSignOnAttributes; import java.util.Arrays; @@ -23,19 +27,76 @@ public class SamlInitiateSingleSignOnRequestTests extends ESTestCase { - public void testSerialization() throws Exception { + public void testSerializationCurrentVersion() throws Exception { final SamlInitiateSingleSignOnRequest request = new SamlInitiateSingleSignOnRequest(); request.setSpEntityId("https://kibana_url"); request.setAssertionConsumerService("https://kibana_url/acs"); + request.setAttributes( + new SamlInitiateSingleSignOnAttributes( + Map.ofEntries( + Map.entry("http://idp.elastic.co/attribute/custom1", List.of("foo")), + Map.entry("http://idp.elastic.co/attribute/custom2", List.of("bar", "baz")) + ) + ) + ); assertThat("An invalid request is not guaranteed to serialize correctly", request.validate(), nullValue()); final BytesStreamOutput out = new BytesStreamOutput(); + if (randomBoolean()) { + out.setTransportVersion( + TransportVersionUtils.randomVersionBetween( + random(), + TransportVersions.IDP_CUSTOM_SAML_ATTRIBUTES, + TransportVersion.current() + ) + ); + } request.writeTo(out); - final SamlInitiateSingleSignOnRequest request1 = new SamlInitiateSingleSignOnRequest(out.bytes().streamInput()); - assertThat(request1.getSpEntityId(), equalTo(request.getSpEntityId())); - assertThat(request1.getAssertionConsumerService(), equalTo(request.getAssertionConsumerService())); - final ActionRequestValidationException validationException = request1.validate(); - assertNull(validationException); + try (StreamInput in = out.bytes().streamInput()) { + in.setTransportVersion(out.getTransportVersion()); + final SamlInitiateSingleSignOnRequest request1 = new SamlInitiateSingleSignOnRequest(in); + assertThat(request1.getSpEntityId(), equalTo(request.getSpEntityId())); + assertThat(request1.getAssertionConsumerService(), equalTo(request.getAssertionConsumerService())); + assertThat(request1.getAttributes(), equalTo(request.getAttributes())); + final ActionRequestValidationException validationException = request1.validate(); + assertNull(validationException); + } + } + + public void testSerializationOldTransportVersion() throws Exception { + final SamlInitiateSingleSignOnRequest request = new SamlInitiateSingleSignOnRequest(); + request.setSpEntityId("https://kibana_url"); + request.setAssertionConsumerService("https://kibana_url/acs"); + if (randomBoolean()) { + request.setAttributes( + new SamlInitiateSingleSignOnAttributes( + Map.ofEntries( + Map.entry("http://idp.elastic.co/attribute/custom1", List.of("foo")), + Map.entry("http://idp.elastic.co/attribute/custom2", List.of("bar", "baz")) + ) + ) + ); + } + assertThat("An invalid request is not guaranteed to serialize correctly", request.validate(), nullValue()); + final BytesStreamOutput out = new BytesStreamOutput(); + out.setTransportVersion( + TransportVersionUtils.randomVersionBetween( + random(), + TransportVersions.MINIMUM_COMPATIBLE, + TransportVersionUtils.getPreviousVersion(TransportVersions.IDP_CUSTOM_SAML_ATTRIBUTES) + ) + ); + request.writeTo(out); + + try (StreamInput in = out.bytes().streamInput()) { + in.setTransportVersion(out.getTransportVersion()); + final SamlInitiateSingleSignOnRequest request1 = new SamlInitiateSingleSignOnRequest(in); + assertThat(request1.getSpEntityId(), equalTo(request.getSpEntityId())); + assertThat(request1.getAssertionConsumerService(), equalTo(request.getAssertionConsumerService())); + assertThat(request1.getAttributes(), nullValue()); + final ActionRequestValidationException validationException = request1.validate(); + assertNull(validationException); + } } public void testValidation() {