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

Add new DefinedBy property to authenticator config. #5938

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

package org.wso2.carbon.identity.application.common;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.wso2.carbon.identity.application.common.model.FederatedAuthenticatorConfig;
import org.wso2.carbon.identity.application.common.model.LocalAuthenticatorConfig;
import org.wso2.carbon.identity.application.common.model.RequestPathAuthenticatorConfig;
Expand All @@ -31,6 +33,7 @@
public class ApplicationAuthenticatorService {

private static volatile ApplicationAuthenticatorService instance;
private static final Log LOG = LogFactory.getLog(ApplicationAuthenticatorService.class);

private List<LocalAuthenticatorConfig> localAuthenticators = new ArrayList<>();
private List<FederatedAuthenticatorConfig> federatedAuthenticators = new ArrayList<>();
Expand Down Expand Up @@ -88,6 +91,10 @@ public RequestPathAuthenticatorConfig getRequestPathAuthenticatorByName(String n

public void addLocalAuthenticator(LocalAuthenticatorConfig authenticator) {
if (authenticator != null) {
//TODO: Remove warn log, once feature is ready.
if (authenticator.getDefinedByType() == null) {
LOG.warn("The defined by type is not set for the : " + authenticator.getName());
}
Copy link
Member

Choose a reason for hiding this comment

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

Well, IMO this is not a good practice to follow.
We clearly know that this can be null for exisitng authenticators, so we should move to the default which is system

localAuthenticators.add(authenticator);
}
}
Expand All @@ -100,6 +107,10 @@ public void removeLocalAuthenticator(LocalAuthenticatorConfig authenticator) {

public void addFederatedAuthenticator(FederatedAuthenticatorConfig authenticator) {
if (authenticator != null) {
//TODO: Remove warn log, once feature is ready.
if (authenticator.getDefinedByType() == null) {
LOG.warn("The defined by type is not set for the : " + authenticator.getName());
}
federatedAuthenticators.add(authenticator);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import org.apache.axiom.om.OMElement;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.wso2.carbon.identity.base.IdentityConstants;

import java.io.Serializable;
import java.util.ArrayList;
Expand All @@ -46,6 +49,7 @@
public class FederatedAuthenticatorConfig implements Serializable {

private static final long serialVersionUID = -2361107623257323257L;
private static final Logger LOG = LoggerFactory.getLogger(LocalAuthenticatorConfig.class);

@XmlElement(name = "Name")
protected String name;
Expand All @@ -63,6 +67,9 @@ public class FederatedAuthenticatorConfig implements Serializable {
@XmlElement(name = "Tags")
protected String[] tags;

@XmlElement(name = "DefinedBy")
protected IdentityConstants.DefinedByType definedByType;

public static FederatedAuthenticatorConfig build(OMElement federatedAuthenticatorConfigOM) {

if (federatedAuthenticatorConfigOM == null) {
Expand Down Expand Up @@ -101,9 +108,18 @@ public static FederatedAuthenticatorConfig build(OMElement federatedAuthenticato
Property[] propertiesArr = propertiesArrList.toArray(new Property[propertiesArrList.size()]);
federatedAuthenticatorConfig.setProperties(propertiesArr);
}
} else if ("DefinedBy".equals(elementName)) {
federatedAuthenticatorConfig.setDefinedByType(
IdentityConstants.DefinedByType.valueOf(element.getText()));
}
}

// TODO: Remove warn log, once feature is ready.
if (federatedAuthenticatorConfig.getDefinedByType() == null) {
federatedAuthenticatorConfig.setDefinedByType(IdentityConstants.DefinedByType.SYSTEM);
LOG.warn("The defined by type is not set for the : " + federatedAuthenticatorConfig.getName());
Copy link
Member

Choose a reason for hiding this comment

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

Let's have a debug log here rather than a warn. Also, let's indicate that 'system' is set by default

}

return federatedAuthenticatorConfig;
}

Expand Down Expand Up @@ -230,4 +246,24 @@ public void setTags(String[] tagList) {

tags = tagList;
}

/**
* Get the tag list of the Local authenticator.
*
* @return String[]
*/
public IdentityConstants.DefinedByType getDefinedByType() {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this DefinedByType enum defined relative to application authenticator than using IdentityConstants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot do this, as it cause circular dependency.


return definedByType;
}

/**
* Set the tag list for Local authenticator config.
*
* @param type authenticator.
*/
public void setDefinedByType(IdentityConstants.DefinedByType type) {

definedByType = type;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
public class IdentityProvider implements Serializable {

private static final long serialVersionUID = 2199048941051702943L;
private static final Log LOG = LogFactory.getLog(IdentityProvider.class);

private static final Log log = LogFactory.getLog(IdentityProvider.class);
private static final String FILE_ELEMENT_IDENTITY_PROVIDER_NAME = "IdentityProviderName";
Expand Down Expand Up @@ -419,6 +420,13 @@ public void setFederatedAuthenticatorConfigs(
if (federatedAuthenticatorConfigs == null) {
return;
}

// TODO: Remove warn log, once feature is ready.
for (FederatedAuthenticatorConfig config: federatedAuthenticatorConfigs) {
if (config.getDefinedByType() == null) {
LOG.warn("The defined by type is not set for the : " + config.getName());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

same comment above applies here

Set<FederatedAuthenticatorConfig> propertySet =
new HashSet<FederatedAuthenticatorConfig>(Arrays.asList(federatedAuthenticatorConfigs));
this.federatedAuthenticatorConfigs = propertySet.toArray(new FederatedAuthenticatorConfig[propertySet.size()]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import org.apache.axiom.om.OMElement;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.wso2.carbon.identity.base.IdentityConstants;

import java.io.Serializable;
Expand All @@ -46,6 +48,7 @@
public class LocalAuthenticatorConfig implements Serializable {

private static final long serialVersionUID = 3363298518257599291L;
private static final Logger LOG = LoggerFactory.getLogger(LocalAuthenticatorConfig.class);

@XmlElement(name = "Name")
protected String name;
Expand All @@ -63,6 +66,9 @@ public class LocalAuthenticatorConfig implements Serializable {
@XmlElement(name = "Tags")
protected String[] tags;

@XmlElement(name = "DefinedBy")
protected IdentityConstants.DefinedByType definedByType;

/*
* <LocalAuthenticatorConfig> <Name></Name> <DisplayName></DisplayName> <IsEnabled></IsEnabled>
* <Properties></Properties> </LocalAuthenticatorConfig>
Expand Down Expand Up @@ -111,8 +117,16 @@ public static LocalAuthenticatorConfig build(OMElement localAuthenticatorConfigO
Property[] propertiesArr = propertiesArrList.toArray(new Property[0]);
localAuthenticatorConfig.setProperties(propertiesArr);
}
} else if ("DefinedBy".equals(member.getLocalName())) {
localAuthenticatorConfig.setDefinedByType(IdentityConstants.DefinedByType.valueOf(member.getText()));
}
}

if (localAuthenticatorConfig.getDefinedByType() == null) {
localAuthenticatorConfig.setDefinedByType(IdentityConstants.DefinedByType.SYSTEM);
LOG.warn("The defined by type is not set for the : " + localAuthenticatorConfig.getName());
}

return localAuthenticatorConfig;
}

Expand Down Expand Up @@ -224,4 +238,24 @@ public void setTags(String[] tagList) {

tags = tagList;
}

/**
* Get the tag list of the Local authenticator.
*
* @return String[]
*/
public IdentityConstants.DefinedByType getDefinedByType() {

return definedByType;
}

/**
* Set the tag list for Local authenticator config.
*
* @param type authenticator.
*/
public void setDefinedByType(IdentityConstants.DefinedByType type) {

definedByType = type;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ private ApplicationConstants() {
public static final String IDP_NAME = "idpName";
public static final String IDP_AUTHENTICATOR_NAME = "authenticatorName";
public static final String IDP_AUTHENTICATOR_DISPLAY_NAME = "authenticatorDisplayName";
public static final String IDP_AUTHENTICATOR_DEFINED_BY_TYPE = "definedByType";
public static final String APPLICATION_DOMAIN = "Application";
// Regex for validating application name.
public static final String APP_NAME_VALIDATING_REGEX = "^[a-zA-Z0-9 ._-]*$";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
import org.wso2.carbon.identity.application.mgt.dao.PaginatableFilterableApplicationDAO;
import org.wso2.carbon.identity.application.mgt.internal.ApplicationManagementServiceComponent;
import org.wso2.carbon.identity.application.mgt.internal.ApplicationManagementServiceComponentHolder;
import org.wso2.carbon.identity.base.IdentityConstants;
import org.wso2.carbon.identity.base.IdentityException;
import org.wso2.carbon.identity.base.IdentityRuntimeException;
import org.wso2.carbon.identity.core.CertificateRetrievingException;
Expand Down Expand Up @@ -1566,6 +1567,15 @@ private void updateLocalAndOutboundAuthenticationConfiguration(int applicationId
ApplicationConstants.LOCAL_IDP_NAME,
lclAuthenticator.getName(),
lclAuthenticator.getDisplayName());
} else {
if (lclAuthenticator.getDefinedByType() == null) {
log.warn("Authenticator already exists. Updating the authenticator, but the " +
"defined by type is not set.");
} else {
log.debug("Authenticator already exists. Updating the authenticator.The defined "
+ "by type is set to: " + lclAuthenticator.getDefinedByType().toString());
//TODO: Update database with defined by properties for local authenticators.
}
}
if (authenticatorId > 0) {
// ID, TENANT_ID, AUTHENTICATOR_ID
Expand Down Expand Up @@ -3088,6 +3098,8 @@ private LocalAndOutboundAuthenticationConfig getLocalAndOutboundAuthenticationCo
.get(ApplicationConstants.IDP_AUTHENTICATOR_NAME));
localAuthenticator.setDisplayName(authenticatorInfo
.get(ApplicationConstants.IDP_AUTHENTICATOR_DISPLAY_NAME));
localAuthenticator.setDefinedByType(IdentityConstants.DefinedByType.valueOf(
authenticatorInfo.get(ApplicationConstants.IDP_AUTHENTICATOR_DEFINED_BY_TYPE)));
stepLocalAuth.get(step).add(localAuthenticator);
} else {
Map<String, List<FederatedAuthenticatorConfig>> stepFedIdps = stepFedIdPAuthenticators
Expand All @@ -3106,6 +3118,8 @@ private LocalAndOutboundAuthenticationConfig getLocalAndOutboundAuthenticationCo
.get(ApplicationConstants.IDP_AUTHENTICATOR_NAME));
fedAuthenticator.setDisplayName(authenticatorInfo
.get(ApplicationConstants.IDP_AUTHENTICATOR_DISPLAY_NAME));
fedAuthenticator.setDefinedByType(IdentityConstants.DefinedByType.valueOf(
authenticatorInfo.get(ApplicationConstants.IDP_AUTHENTICATOR_DEFINED_BY_TYPE)));
idpAuths.add(fedAuthenticator);
}

Expand Down Expand Up @@ -5017,6 +5031,9 @@ private Map<String, String> getAuthenticatorInfo(Connection conn, int tenantId,
returnData
.put(ApplicationConstants.IDP_AUTHENTICATOR_DISPLAY_NAME, rs.getString(3));
}
// TODO: Read from database and set the DefinedBy property to the authenticator.
returnData.put(ApplicationConstants.IDP_AUTHENTICATOR_DEFINED_BY_TYPE,
IdentityConstants.DefinedByType.SYSTEM.toString());
} finally {
IdentityApplicationManagementUtil.closeStatement(prepStmt);
}
Expand All @@ -5038,7 +5055,7 @@ private int addAuthenticator(Connection conn, int tenantId, String idpName,
int authenticatorId = -1;
PreparedStatement prepStmt = null;
ResultSet rs = null;
// TENANT_ID, IDP_ID, NAME,IS_ENABLED, DISPLAY_NAME
// TENANT_ID, IDP_ID, NAME,IS_ENABLED, DISPLAY_NAME, DEFINED_BY
String sqlStmt = ApplicationMgtDBQueries.STORE_LOCAL_AUTHENTICATOR;
try {
String dbProductName = conn.getMetaData().getDatabaseProductName();
Expand All @@ -5050,6 +5067,7 @@ private int addAuthenticator(Connection conn, int tenantId, String idpName,
prepStmt.setString(4, authenticatorName);
prepStmt.setString(5, "1");
prepStmt.setString(6, authenticatorDispalyName);
//TODO: prepStmt.setString(7, IdentityConstants.DefinedByType.SYSTEM.toString());
prepStmt.execute();
rs = prepStmt.getGeneratedKeys();
if (rs.next()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,19 @@ public void testGetConfiguredAuthenticators() throws IdentityApplicationManageme
AuthenticationStep[] steps = applicationManagementService.getConfiguredAuthenticators(resourceID,
SUPER_TENANT_DOMAIN_NAME);

for (AuthenticationStep step : steps) {
LocalAuthenticatorConfig[] localAuthenticators = step.getLocalAuthenticatorConfigs();
for (LocalAuthenticatorConfig localConfig : localAuthenticators) {
Assert.assertNotNull(localConfig.getDefinedByType());
}
IdentityProvider[] identityProviders = step.getFederatedIdentityProviders();
for (IdentityProvider idp : identityProviders) {
for (FederatedAuthenticatorConfig fedConfig: idp.getFederatedAuthenticatorConfigs()) {
Assert.assertNotNull(fedConfig.getDefinedByType());
}
}
}

Assert.assertEquals(steps.length, 1);
Assert.assertEquals(steps[0].getStepOrder(), 1);
applicationManagementService.deleteApplication(APPLICATION_NAME_1, SUPER_TENANT_DOMAIN_NAME, USERNAME_1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.wso2.carbon.identity.application.authentication.framework.exception.LogoutFailedException;
import org.wso2.carbon.identity.application.authentication.framework.model.AuthenticatorData;
import org.wso2.carbon.identity.application.common.model.Property;
import org.wso2.carbon.identity.base.IdentityConstants;

import java.io.Serializable;
import java.util.List;
Expand Down Expand Up @@ -171,4 +172,13 @@ default String getI18nKey() {
return StringUtils.EMPTY;
}

/**
* Get the authenticator type. Default value will be SYSTEM.
*
* @return Authenticator Type.
*/
default IdentityConstants.DefinedByType getDefinedByType() {

return IdentityConstants.DefinedByType.SYSTEM;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
import org.wso2.carbon.identity.application.common.model.Property;
import org.wso2.carbon.identity.application.common.model.RequestPathAuthenticatorConfig;
import org.wso2.carbon.identity.application.mgt.ApplicationManagementService;
import org.wso2.carbon.identity.base.IdentityConstants;
import org.wso2.carbon.identity.claim.metadata.mgt.ClaimMetadataManagementService;
import org.wso2.carbon.identity.configuration.mgt.core.ConfigurationManager;
import org.wso2.carbon.identity.core.handler.HandlerComparator;
Expand Down Expand Up @@ -506,6 +507,7 @@ protected void setAuthenticator(ApplicationAuthenticator authenticator) {
localAuthenticatorConfig.setProperties(configProperties);
localAuthenticatorConfig.setDisplayName(authenticator.getFriendlyName());
localAuthenticatorConfig.setTags(getTags(authenticator));
localAuthenticatorConfig.setDefinedByType(IdentityConstants.DefinedByType.SYSTEM);
AuthenticatorConfig fileBasedConfig = getAuthenticatorConfig(authenticator.getName());
localAuthenticatorConfig.setEnabled(fileBasedConfig.isEnabled());
ApplicationAuthenticatorService.getInstance().addLocalAuthenticator(localAuthenticatorConfig);
Expand All @@ -515,6 +517,7 @@ protected void setAuthenticator(ApplicationAuthenticator authenticator) {
federatedAuthenticatorConfig.setProperties(configProperties);
federatedAuthenticatorConfig.setDisplayName(authenticator.getFriendlyName());
federatedAuthenticatorConfig.setTags(getTags(authenticator));
federatedAuthenticatorConfig.setDefinedByType(IdentityConstants.DefinedByType.SYSTEM);
ApplicationAuthenticatorService.getInstance().addFederatedAuthenticator(federatedAuthenticatorConfig);
} else if (authenticator instanceof RequestPathApplicationAuthenticator) {
RequestPathAuthenticatorConfig reqPathAuthenticatorConfig = new RequestPathAuthenticatorConfig();
Expand All @@ -524,6 +527,7 @@ protected void setAuthenticator(ApplicationAuthenticator authenticator) {
reqPathAuthenticatorConfig.setTags(getTags(authenticator));
AuthenticatorConfig fileBasedConfig = getAuthenticatorConfig(authenticator.getName());
reqPathAuthenticatorConfig.setEnabled(fileBasedConfig.isEnabled());
reqPathAuthenticatorConfig.setDefinedByType(IdentityConstants.DefinedByType.SYSTEM);
ApplicationAuthenticatorService.getInstance().addRequestPathAuthenticator(reqPathAuthenticatorConfig);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@
import org.wso2.carbon.identity.application.common.model.Property;
import org.wso2.carbon.identity.application.common.model.ServiceProvider;
import org.wso2.carbon.identity.application.mgt.ApplicationConstants;
import org.wso2.carbon.identity.base.IdentityConstants;
import org.wso2.carbon.identity.base.IdentityException;
import org.wso2.carbon.identity.base.IdentityRuntimeException;
import org.wso2.carbon.identity.central.log.mgt.utils.LoggerUtils;
Expand Down Expand Up @@ -4190,4 +4191,23 @@ public static boolean isURLRelative(String uriString) throws URISyntaxException

return !new URI(uriString).isAbsolute();
}

/**
* This method return defined by type for the given authenticator name.
*
* @param authenticatorName Name of the authenticator.
* @return The defined by type.
* @throws FrameworkException If no authenticator found for the given authenticator name.
*/
public static IdentityConstants.DefinedByType getAuthenticatorDefinedByType(String authenticatorName)
throws FrameworkException {

for (ApplicationAuthenticator authenticator: FrameworkServiceComponent.getAuthenticators()) {
if (authenticator.getName().equals(authenticatorName)) {
return authenticator.getDefinedByType();
}
}

throw new FrameworkException("No authenticator instance is found for " + authenticatorName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -619,4 +619,13 @@ public static class APIResponse {

public static final String SET_ACCOUNT_LOCK_AUTH_FAILURE_REASON = "APIResponse.SetAccountLockAuthFailureReason";
}

/**
* The Authentication Type - SYSTEM: system define authenticator, CUSTOM: user defined authentication extension.
*/
public enum DefinedByType {

SYSTEM,
USER
}
}
Loading
Loading