Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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.
*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is ideal to use block comment (/* */) in these places

userAttributeSeparator = FrameworkUtils.getMultiAttributeSeparator();
}
claims.remove(IdentityCoreConstants.MULTI_ATTRIBUTE_SEPARATOR);
claims.remove(FrameworkConstants.IDP_MAPPED_USER_ROLES);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -61,6 +62,7 @@
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.Paths;

import javax.servlet.Servlet;

/**
Expand Down Expand Up @@ -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 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* 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 */
PasinduYeshan marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.*"})
Expand Down Expand Up @@ -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 {

Expand Down
Loading