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.

Sign up for GitHub

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

Move SAML SP validation to SAMLSSOServiceProviderManager layer #6263

Closed
wants to merge 1 commit into from
Closed

Move SAML SP validation to SAMLSSOServiceProviderManager layer #6263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
95 changes: 92 additions & 3 deletions ...ntity.core/src/main/java/org/wso2/carbon/identity/core/SAMLSSOServiceProviderManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@

package org.wso2.carbon.identity.core;

import org.apache.commons.lang.StringUtils;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.wso2.carbon.identity.base.IdentityException;
import org.wso2.carbon.identity.core.dao.SAMLServiceProviderPersistenceManagerFactory;
import org.wso2.carbon.identity.core.dao.SAMLSSOServiceProviderDAO;
Expand All @@ -33,6 +36,7 @@ public class SAMLSSOServiceProviderManager {
new SAMLServiceProviderPersistenceManagerFactory();
SAMLSSOServiceProviderDAO serviceProviderDAO =
samlSSOPersistenceManagerFactory.getSAMLServiceProviderPersistenceManager();
private static Log LOG = LogFactory.getLog(SAMLSSOServiceProviderManager.class);

/**
* Add a saml service provider.
Expand All @@ -45,6 +49,13 @@ public class SAMLSSOServiceProviderManager {
public boolean addServiceProvider(SAMLSSOServiceProviderDO serviceProviderDO, int tenantId)
throws IdentityException {

validateServiceProvider(serviceProviderDO);
if (isServiceProviderExists(serviceProviderDO.getIssuer(), tenantId)) {
if (LOG.isDebugEnabled()){
LOG.debug(serviceProviderInfo(serviceProviderDO) + " already exists.");
}
return false;
}
return serviceProviderDAO.addServiceProvider(serviceProviderDO, tenantId);
}

Expand All @@ -60,6 +71,15 @@ public boolean addServiceProvider(SAMLSSOServiceProviderDO serviceProviderDO, in
public boolean updateServiceProvider(SAMLSSOServiceProviderDO serviceProviderDO, String currentIssuer, int tenantId)
throws IdentityException {

validateServiceProvider(serviceProviderDO);
String newIssuer = serviceProviderDO.getIssuer();
boolean isIssuerUpdated = !StringUtils.equals(currentIssuer, newIssuer);
if (isIssuerUpdated && isServiceProviderExists(newIssuer, tenantId)) {
if (LOG.isDebugEnabled()) {
LOG.debug(serviceProviderInfo(serviceProviderDO) + " already exists.");
}
return false;
}
return serviceProviderDAO.updateServiceProvider(serviceProviderDO, currentIssuer, tenantId);
}

Expand Down Expand Up @@ -110,20 +130,89 @@ public boolean isServiceProviderExists(String issuer, int tenantId) throws Ident
*/
public boolean removeServiceProvider(String issuer, int tenantId) throws IdentityException {

if (issuer == null || StringUtils.isEmpty(issuer.trim())) {
throw new IllegalArgumentException("Trying to delete issuer \'" + issuer + "\'");
}
if (!isServiceProviderExists(issuer, tenantId)) {
if (LOG.isDebugEnabled()) {
LOG.debug("Service Provider with issuer: " + issuer + " does not exist.");
}
return false;
}
return serviceProviderDAO.removeServiceProvider(issuer, tenantId);
}

/**
* Upload the SAML configuration related to the application, using metadata.
*
* @param samlssoServiceProviderDO SAML service provider information object.
* @param serviceProviderDO SAML service provider information object.
* @param tenantId Tenant ID.
* @return SAML service provider information object.
* @throws IdentityException Error when uploading the SAML configuration.
*/
public SAMLSSOServiceProviderDO uploadServiceProvider(SAMLSSOServiceProviderDO samlssoServiceProviderDO,
public SAMLSSOServiceProviderDO uploadServiceProvider(SAMLSSOServiceProviderDO serviceProviderDO,
int tenantId) throws IdentityException {

return serviceProviderDAO.uploadServiceProvider(samlssoServiceProviderDO, tenantId);
validateServiceProvider(serviceProviderDO);
if (serviceProviderDO.getDefaultAssertionConsumerUrl() == null) {
throw new IdentityException("No default assertion consumer URL provided for service provider :" +
serviceProviderDO.getIssuer());
}
if (isServiceProviderExists(serviceProviderDO.getIssuer(), tenantId)) {
if (LOG.isDebugEnabled()){
LOG.debug(serviceProviderInfo(serviceProviderDO) + " already exists.");
}
throw new IdentityException("A Service Provider already exists.");
}

return serviceProviderDAO.uploadServiceProvider(serviceProviderDO, tenantId);
}

private void validateServiceProvider(SAMLSSOServiceProviderDO serviceProviderDO) throws IdentityException {

if (serviceProviderDO == null || serviceProviderDO.getIssuer() == null ||
StringUtils.isBlank(serviceProviderDO.getIssuer())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

StringUtils.isBlank(serviceProviderDO.getIssuer() part is not available in the uploadServiceProvider() validation in the RegistrySAMLSSOServiceProviderDAOImpl.

Is that expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added the check to ensure the issuer isnt white space

throw new IdentityException("Issuer cannot be found in the provided arguments.");
}

if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier()) &&
!serviceProviderDO.getIssuer().contains(IdentityRegistryResources.QUALIFIER_ID)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

&& !serviceProviderDO.getIssuer().contains(IdentityRegistryResources.QUALIFIER_ID)
This part is not available in the addServiceProvider() validation in the RegistrySAMLSSOServiceProviderDAOImpl.. Is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same concern is there for uploadServiceProvider() as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We added the check to ensure that the qualifier wont be appended again to the issuer value and set in the DO

serviceProviderDO.setIssuer(
getIssuerWithQualifier(serviceProviderDO.getIssuer(), serviceProviderDO.getIssuerQualifier()));
}
}

private String serviceProviderInfo(SAMLSSOServiceProviderDO serviceProviderDO) {

if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier())) {
return "SAML2 Service Provider with issuer: " + getIssuerWithoutQualifier(serviceProviderDO.getIssuer()) +
" and qualifier name " + serviceProviderDO.getIssuerQualifier();
} else {
return "SAML2 Service Provider with issuer: " + serviceProviderDO.getIssuer();
}
}

/**
* Get the issuer value to be added to registry by appending the qualifier.
*
* @param issuer value given as 'issuer' when configuring SAML SP.
* @return issuer value with qualifier appended.
*/
private String getIssuerWithQualifier(String issuer, String qualifier) {

return issuer + IdentityRegistryResources.QUALIFIER_ID + qualifier;
}

/**
* Get the issuer value by removing the qualifier.
*
* @param issuerWithQualifier issuer value saved in the registry.
* @return issuer value given as 'issuer' when configuring SAML SP.
*/
private String getIssuerWithoutQualifier(String issuerWithQualifier) {

String issuerWithoutQualifier = StringUtils.substringBeforeLast(issuerWithQualifier,
IdentityRegistryResources.QUALIFIER_ID);
return issuerWithoutQualifier;
}
}
89 changes: 0 additions & 89 deletions ...rc/main/java/org/wso2/carbon/identity/core/dao/RegistrySAMLSSOServiceProviderDAOImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,36 +256,11 @@ SAMLSSOServiceProviderDO buildSAMLSSOServiceProviderDAO (Resource resource) {
public boolean addServiceProvider(SAMLSSOServiceProviderDO serviceProviderDO, int tenantId) throws IdentityException {

Registry registry = getRegistry(tenantId);
if (serviceProviderDO == null || serviceProviderDO.getIssuer() == null ||
StringUtils.isBlank(serviceProviderDO.getIssuer())) {
throw new IdentityException("Issuer cannot be found in the provided arguments.");
}

// If an issuer qualifier value is specified, it is appended to the end of the issuer value.
if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier())) {
serviceProviderDO.setIssuer(getIssuerWithQualifier(serviceProviderDO.getIssuer(),
serviceProviderDO.getIssuerQualifier()));
}

String path = IdentityRegistryResources.SAML_SSO_SERVICE_PROVIDERS + encodePath(serviceProviderDO.getIssuer());

boolean isTransactionStarted = Transaction.isStarted();
boolean isErrorOccurred = false;
try {
if (registry.resourceExists(path)) {
if (LOG.isDebugEnabled()) {
if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier())) {
LOG.debug("SAML2 Service Provider already exists with the same issuer name "
+ getIssuerWithoutQualifier(serviceProviderDO.getIssuer()) + " and qualifier name "
+ serviceProviderDO.getIssuerQualifier());
} else {
LOG.debug("SAML2 Service Provider already exists with the same issuer name "
+ serviceProviderDO.getIssuer());
}
}
return false;
}

Resource resource = createResource(serviceProviderDO, registry);
if (!isTransactionStarted) {
registry.beginTransaction();
Expand Down Expand Up @@ -480,39 +455,13 @@ public boolean updateServiceProvider(SAMLSSOServiceProviderDO serviceProviderDO,
throws IdentityException {

Registry registry = getRegistry(tenantId);
if (serviceProviderDO == null || serviceProviderDO.getIssuer() == null ||
StringUtils.isBlank(serviceProviderDO.getIssuer())) {
throw new IdentityException("Issuer cannot be found in the provided arguments.");
}

// If an issuer qualifier value is specified, it is appended to the end of the issuer value.
if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier())) {
serviceProviderDO.setIssuer(getIssuerWithQualifier(serviceProviderDO.getIssuer(),
serviceProviderDO.getIssuerQualifier()));
}

String currentPath = IdentityRegistryResources.SAML_SSO_SERVICE_PROVIDERS + encodePath(currentIssuer);
String newPath = IdentityRegistryResources.SAML_SSO_SERVICE_PROVIDERS + encodePath(serviceProviderDO.getIssuer());

boolean isIssuerUpdated = !StringUtils.equals(currentPath, newPath);
boolean isTransactionStarted = Transaction.isStarted();
boolean isErrorOccurred = false;
try {
// Check if the updated issuer value already exists.
if (isIssuerUpdated && registry.resourceExists(newPath)) {
if (LOG.isDebugEnabled()) {
if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier())) {
LOG.debug("SAML2 Service Provider already exists with the same issuer name "
+ getIssuerWithoutQualifier(serviceProviderDO.getIssuer()) + " and qualifier name "
+ serviceProviderDO.getIssuerQualifier());
} else {
LOG.debug("SAML2 Service Provider already exists with the same issuer name "
+ serviceProviderDO.getIssuer());
}
}
return false;
}

Resource resource = createResource(serviceProviderDO, registry);
if (!isTransactionStarted) {
registry.beginTransaction();
Expand Down Expand Up @@ -578,21 +527,11 @@ public SAMLSSOServiceProviderDO[] getServiceProviders(int tenantId) throws Ident
public boolean removeServiceProvider(String issuer, int tenantId) throws IdentityException {

Registry registry = getRegistry(tenantId);
if (issuer == null || StringUtils.isEmpty(issuer.trim())) {
throw new IllegalArgumentException("Trying to delete issuer \'" + issuer + "\'");
}

String path = IdentityRegistryResources.SAML_SSO_SERVICE_PROVIDERS + encodePath(issuer);
boolean isTransactionStarted = Transaction.isStarted();
boolean isErrorOccurred = false;
try {
if (!registry.resourceExists(path)) {
if (LOG.isDebugEnabled()) {
LOG.debug("Registry resource does not exist for the path: " + path);
}
return false;
}

// Since we are getting a global registry object, better to check whether this is a task inside already
// started transaction.
if (!isTransactionStarted) {
Expand Down Expand Up @@ -738,39 +677,11 @@ public SAMLSSOServiceProviderDO uploadServiceProvider(SAMLSSOServiceProviderDO s
throws IdentityException {

Registry registry = getRegistry(tenantId);
if (serviceProviderDO == null || serviceProviderDO.getIssuer() == null) {
throw new IdentityException("Issuer cannot be found in the provided arguments.");
}

if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier())) {
serviceProviderDO.setIssuer(getIssuerWithQualifier(serviceProviderDO.getIssuer(),
serviceProviderDO.getIssuerQualifier()));
}

if (serviceProviderDO.getDefaultAssertionConsumerUrl() == null) {
throw new IdentityException("No default assertion consumer URL provided for service provider :" +
serviceProviderDO.getIssuer());
}

String path = IdentityRegistryResources.SAML_SSO_SERVICE_PROVIDERS + encodePath(serviceProviderDO.getIssuer());

boolean isTransactionStarted = Transaction.isStarted();
boolean isErrorOccurred = false;
try {
if (registry.resourceExists(path)) {
if (LOG.isDebugEnabled()) {
if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier())) {
LOG.debug("SAML2 Service Provider already exists with the same issuer name "
+ getIssuerWithoutQualifier(serviceProviderDO.getIssuer()) + " and qualifier name "
+ serviceProviderDO.getIssuerQualifier());
} else {
LOG.debug("SAML2 Service Provider already exists with the same issuer name "
+ serviceProviderDO.getIssuer());
}
}
throw IdentityException.error("A Service Provider already exists.");
}

if (!isTransactionStarted) {
registry.beginTransaction();
}
Expand Down
23 changes: 10 additions & 13 deletions ...e/src/main/java/org/wso2/carbon/identity/core/persistence/IdentityPersistenceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,10 @@

import org.wso2.carbon.context.CarbonContext;
import org.wso2.carbon.identity.base.IdentityException;
import org.wso2.carbon.identity.core.SAMLSSOServiceProviderManager;
import org.wso2.carbon.identity.core.dao.OpenIDAdminDAO;
import org.wso2.carbon.identity.core.dao.OpenIDUserDAO;
import org.wso2.carbon.identity.core.dao.ParameterDAO;
import org.wso2.carbon.identity.core.dao.SAMLServiceProviderPersistenceManagerFactory;
import org.wso2.carbon.identity.core.dao.SAMLSSOServiceProviderDAO;
import org.wso2.carbon.identity.core.dao.XMPPSettingsDAO;
import org.wso2.carbon.identity.core.model.OpenIDAdminDO;
import org.wso2.carbon.identity.core.model.OpenIDUserDO;
Expand All @@ -36,9 +35,7 @@
public class IdentityPersistenceManager {

private static IdentityPersistenceManager manager = new IdentityPersistenceManager();
SAMLServiceProviderPersistenceManagerFactory
samlSSOPersistenceManagerFactory = new SAMLServiceProviderPersistenceManagerFactory();
SAMLSSOServiceProviderDAO serviceProviderDAO = samlSSOPersistenceManagerFactory.getSAMLServiceProviderPersistenceManager();
SAMLSSOServiceProviderManager samlSSOServiceProviderManager = new SAMLSSOServiceProviderManager();

private IdentityPersistenceManager() {
}
Expand Down Expand Up @@ -242,20 +239,20 @@ public boolean addServiceProvider(Registry registry, SAMLSSOServiceProviderDO se
throws IdentityException {

int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId();
return serviceProviderDAO.addServiceProvider(serviceProviderDO, tenantId);
return samlSSOServiceProviderManager.addServiceProvider(serviceProviderDO, tenantId);
}

/**
* Upload Service Provider
*
* @param registry,samlssoServiceProviderDO
* @param registry, samlSSOServiceProviderDO
* @return
* @throws IdentityException
*/
public SAMLSSOServiceProviderDO uploadServiceProvider(Registry registry, SAMLSSOServiceProviderDO samlssoServiceProviderDO) throws IdentityException {
public SAMLSSOServiceProviderDO uploadServiceProvider(Registry registry, SAMLSSOServiceProviderDO samlSSOServiceProviderDO) throws IdentityException {

int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId();
return serviceProviderDAO.uploadServiceProvider(samlssoServiceProviderDO, tenantId);
return samlSSOServiceProviderManager.uploadServiceProvider(samlSSOServiceProviderDO, tenantId);
}

/**
Expand All @@ -268,26 +265,26 @@ public SAMLSSOServiceProviderDO[] getServiceProviders(Registry registry)
throws IdentityException {

int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId();
return serviceProviderDAO.getServiceProviders(tenantId);
return samlSSOServiceProviderManager.getServiceProviders(tenantId);
}

public boolean removeServiceProvider(Registry registry, String issuer) throws IdentityException {

int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId();
return serviceProviderDAO.removeServiceProvider(issuer, tenantId);
return samlSSOServiceProviderManager.removeServiceProvider(issuer, tenantId);
}

public SAMLSSOServiceProviderDO getServiceProvider(Registry registry, String issuer)
throws IdentityException {

int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId();
return serviceProviderDAO.getServiceProvider(issuer, tenantId);
return samlSSOServiceProviderManager.getServiceProvider(issuer, tenantId);
}

public boolean isServiceProviderExists(Registry registry, String issuer) throws IdentityException {

int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId();
return serviceProviderDAO.isServiceProviderExists(issuer, tenantId);
return samlSSOServiceProviderManager.isServiceProviderExists(issuer, tenantId);
}

public void createOrUpdateOpenIDAdmin(Registry registry, OpenIDAdminDO opAdmin)
Expand Down
Loading
Loading