Merge pull request #421 from PasinduYeshan/fix/saml-attribute-separator

Fix mutli-valued claims not separating in the SAML federation flow
wso2-extensions · Mar 18, 2024 · a74fbd2 · a74fbd2
2 parents 20eb788 + 1c234b8
commit a74fbd2
Show file tree

Hide file tree

Showing 3 changed files with 45 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,17 @@ 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 {