Fix mutli-valued claims not separating in the SAML federation flow

wso2-extensions · Mar 8, 2024 · 0dde94a · 0dde94a
1 parent 8167e29
commit 0dde94a
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 5 deletions.
diff --git a/...ava/org/wso2/carbon/identity/sso/saml/builders/assertion/DefaultSAMLAssertionBuilder.java b/...ava/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<String, String> 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/...main/java/org/wso2/carbon/identity/sso/saml/internal/IdentitySAMLSSOServiceComponent.java b/...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/....sso.saml/src/test/java/org/wso2/carbon/identity/sso/saml/util/AssertionBuildingTest.java b/....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 {