From 0dde94af903f97151497067a0d007a69f106dff8 Mon Sep 17 00:00:00 2001 From: Pasindu Yeshan Date: Fri, 8 Mar 2024 14:57:59 +0530 Subject: [PATCH 1/3] Fix mutli-valued claims not separating in the SAML federation flow --- .../DefaultSAMLAssertionBuilder.java | 14 +++++++++---- .../IdentitySAMLSSOServiceComponent.java | 21 +++++++++++++++++++ .../sso/saml/util/AssertionBuildingTest.java | 13 +++++++++++- 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java index caa2dcae7..22bd5a7ea 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java @@ -22,6 +22,8 @@ import org.apache.commons.logging.LogFactory; import org.joda.time.DateTime; import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; +import org.opensaml.core.xml.schema.XSString; +import org.opensaml.core.xml.schema.impl.XSStringBuilder; import org.opensaml.saml.common.SAMLVersion; import org.opensaml.saml.saml1.core.NameIdentifier; import org.opensaml.saml.saml2.core.Assertion; @@ -52,10 +54,9 @@ import org.opensaml.saml.saml2.core.impl.SubjectBuilder; import org.opensaml.saml.saml2.core.impl.SubjectConfirmationBuilder; import org.opensaml.saml.saml2.core.impl.SubjectConfirmationDataBuilder; -import org.opensaml.core.xml.schema.XSString; -import org.opensaml.core.xml.schema.impl.XSStringBuilder; import org.wso2.carbon.identity.application.authentication.framework.model.AuthenticationContextProperty; import org.wso2.carbon.identity.application.authentication.framework.util.FrameworkConstants; +import org.wso2.carbon.identity.application.authentication.framework.util.FrameworkUtils; import org.wso2.carbon.identity.application.common.util.IdentityApplicationConstants; import org.wso2.carbon.identity.base.IdentityConstants; import org.wso2.carbon.identity.base.IdentityException; @@ -77,8 +78,6 @@ public class DefaultSAMLAssertionBuilder implements SAMLAssertionBuilder { private static final Log log = LogFactory.getLog(DefaultSAMLAssertionBuilder.class); - private String userAttributeSeparator = IdentityCoreConstants.MULTI_ATTRIBUTE_SEPARATOR_DEFAULT; - @Override public void init() throws IdentityException { //Overridden method, no need to implement the body @@ -339,8 +338,15 @@ private AuthnStatement getAuthnStatement(SAMLSSOAuthnReqDTO authReqDTO, String s protected AttributeStatement buildAttributeStatement(Map claims) { String claimSeparator = claims.get(IdentityCoreConstants.MULTI_ATTRIBUTE_SEPARATOR); + String userAttributeSeparator; if (StringUtils.isNotBlank(claimSeparator)) { userAttributeSeparator = claimSeparator; + } else { + // In the SAML outbound authenticator, multivalued attributes are concatenated using the primary user + // store's attribute separator. Therefore, to ensure uniformity, the multi-attribute separator from + // the primary user store is utilized for separating multivalued attributes when MultiAttributeSeparator + // is not available in the claims. + userAttributeSeparator = FrameworkUtils.getMultiAttributeSeparator(); } claims.remove(IdentityCoreConstants.MULTI_ATTRIBUTE_SEPARATOR); claims.remove(FrameworkConstants.IDP_MAPPED_USER_ROLES); diff --git a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java index dfa809962..deead6a80 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java @@ -30,6 +30,7 @@ import org.osgi.service.component.annotations.ReferencePolicy; import org.osgi.service.http.HttpService; import org.wso2.carbon.base.api.ServerConfigurationService; +import org.wso2.carbon.identity.application.authentication.framework.ApplicationAuthenticationService; import org.wso2.carbon.identity.application.authentication.framework.listener.SessionContextMgtListener; import org.wso2.carbon.identity.application.mgt.ApplicationManagementService; import org.wso2.carbon.identity.application.mgt.inbound.protocol.ApplicationInboundAuthConfigHandler; @@ -61,6 +62,7 @@ import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.Paths; + import javax.servlet.Servlet; /** @@ -465,4 +467,23 @@ protected void unsetSAMLSSOServiceProviderManager(SAMLSSOServiceProviderManager log.debug("SAMLSSOServiceProviderManager unset in to bundle"); } } + + @Reference( + name = "identity.application.authentication.framework", + service = ApplicationAuthenticationService.class, + cardinality = ReferenceCardinality.MANDATORY, + policy = ReferencePolicy.DYNAMIC, + unbind = "unsetApplicationAuthenticationService" + ) + protected void setApplicationAuthenticationService( + ApplicationAuthenticationService applicationAuthenticationService) { + /* reference ApplicationAuthenticationService service to guarantee that this component will wait until + authentication framework is started */ + } + + protected void unsetApplicationAuthenticationService( + ApplicationAuthenticationService applicationAuthenticationService) { + /* reference ApplicationAuthenticationService service to guarantee that this component will wait until + authentication framework is started */ + } } diff --git a/components/org.wso2.carbon.identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/util/AssertionBuildingTest.java b/components/org.wso2.carbon.identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/util/AssertionBuildingTest.java index 7fa2d7ac6..bc61908f1 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/util/AssertionBuildingTest.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/util/AssertionBuildingTest.java @@ -34,10 +34,12 @@ import org.powermock.modules.testng.PowerMockObjectFactory; import org.powermock.modules.testng.PowerMockTestCase; import org.testng.IObjectFactory; +import org.testng.annotations.BeforeMethod; import org.testng.annotations.DataProvider; import org.testng.annotations.ObjectFactory; import org.testng.annotations.Test; import org.wso2.carbon.core.util.KeyStoreManager; +import org.wso2.carbon.identity.application.authentication.framework.util.FrameworkUtils; import org.wso2.carbon.identity.application.common.model.FederatedAuthenticatorConfig; import org.wso2.carbon.identity.application.common.model.IdentityProvider; import org.wso2.carbon.identity.application.common.model.Property; @@ -81,11 +83,13 @@ import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; +import static org.wso2.carbon.identity.core.util.IdentityCoreConstants.MULTI_ATTRIBUTE_SEPARATOR_DEFAULT; + /** * Tests Assertion building functionality. */ @PrepareForTest({IdentityUtil.class, IdentityTenantUtil.class, IdentityProviderManager.class, - SSOServiceProviderConfigManager.class, IdentitySAMLSSOServiceComponentHolder.class}) + SSOServiceProviderConfigManager.class, IdentitySAMLSSOServiceComponentHolder.class, FrameworkUtils.class}) @WithCarbonHome @PowerMockIgnore({"javax.net.*", "javax.xml.*", "org.xml.*", "org.w3c.dom.*", "javax.security.*", "org.mockito.*"}) @@ -126,6 +130,13 @@ public IObjectFactory getObjectFactory() { @Mock private X509Credential x509Credential; + @BeforeMethod + public void setUpBeforeMethod() throws Exception { + + mockStatic(FrameworkUtils.class); + when(FrameworkUtils.getMultiAttributeSeparator()).thenReturn(MULTI_ATTRIBUTE_SEPARATOR_DEFAULT); + } + @Test public void testBuildAssertion() throws Exception { From 3036d5ae2dc96e274aab059ed5d27d83c405e102 Mon Sep 17 00:00:00 2001 From: Pasindu Yeshan Date: Mon, 11 Mar 2024 11:50:42 +0530 Subject: [PATCH 2/3] Refactor --- .../assertion/DefaultSAMLAssertionBuilder.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java index 22bd5a7ea..4ecacf97d 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java @@ -341,11 +341,13 @@ protected AttributeStatement buildAttributeStatement(Map claims) String userAttributeSeparator; if (StringUtils.isNotBlank(claimSeparator)) { userAttributeSeparator = claimSeparator; - } else { - // In the SAML outbound authenticator, multivalued attributes are concatenated using the primary user - // store's attribute separator. Therefore, to ensure uniformity, the multi-attribute separator from - // the primary user store is utilized for separating multivalued attributes when MultiAttributeSeparator - // is not available in the claims. + } else { + /* + * In the SAML outbound authenticator, multivalued attributes are concatenated using the primary user + * store's attribute separator. Therefore, to ensure uniformity, the multi-attribute separator from + * the primary user store is utilized for separating multivalued attributes when MultiAttributeSeparator + * is not available in the claims. + */ userAttributeSeparator = FrameworkUtils.getMultiAttributeSeparator(); } claims.remove(IdentityCoreConstants.MULTI_ATTRIBUTE_SEPARATOR); From 1c234b846943a18bc0b78e41fdfdec65677cfea9 Mon Sep 17 00:00:00 2001 From: Pasindu Yeshan Date: Mon, 18 Mar 2024 11:43:38 +0530 Subject: [PATCH 3/3] Refactor --- .../saml/internal/IdentitySAMLSSOServiceComponent.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java index deead6a80..6a78325b8 100644 --- a/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java +++ b/components/org.wso2.carbon.identity.sso.saml/src/main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java @@ -477,13 +477,13 @@ protected void unsetSAMLSSOServiceProviderManager(SAMLSSOServiceProviderManager ) protected void setApplicationAuthenticationService( ApplicationAuthenticationService applicationAuthenticationService) { - /* reference ApplicationAuthenticationService service to guarantee that this component will wait until - authentication framework is started */ + /* Reference ApplicationAuthenticationService service to guarantee that this component will wait until + authentication framework is started. */ } protected void unsetApplicationAuthenticationService( ApplicationAuthenticationService applicationAuthenticationService) { - /* reference ApplicationAuthenticationService service to guarantee that this component will wait until - authentication framework is started */ + /* Reference ApplicationAuthenticationService service to guarantee that this component will wait until + authentication framework is started. */ } }