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

Improve authentication name validation. #6288

Merged
Show file tree
Hide file tree
Changes from all 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 @@ -38,7 +38,6 @@

import static org.wso2.carbon.identity.application.common.util.AuthenticatorMgtExceptionBuilder.buildClientException;
import static org.wso2.carbon.identity.application.common.util.AuthenticatorMgtExceptionBuilder.buildRuntimeServerException;
import static org.wso2.carbon.identity.application.common.util.IdentityApplicationConstants.Authenticator.DISPLAY_NAME;

/**
* Application authenticator service.
Expand Down Expand Up @@ -207,13 +206,12 @@ public UserDefinedLocalAuthenticatorConfig addUserDefinedLocalAuthenticator(
UserDefinedLocalAuthenticatorConfig authenticatorConfig, String tenantDomain)
throws AuthenticatorMgtException {

LocalAuthenticatorConfig config = getLocalAuthenticatorByName(authenticatorConfig.getName(), tenantDomain);
if (config != null) {
if (isExistingAuthenticatorName(authenticatorConfig.getName(), tenantDomain)) {
throw buildClientException(AuthenticatorMgtError.ERROR_AUTHENTICATOR_ALREADY_EXIST,
authenticatorConfig.getName());
}
authenticatorValidator.validateAuthenticatorName(authenticatorConfig.getName());
authenticatorValidator.validateForBlank(DISPLAY_NAME, authenticatorConfig.getDisplayName());
authenticatorValidator.validateDisplayName(authenticatorConfig.getDisplayName());
if (authenticatorConfig.getImageUrl() != null) {
authenticatorValidator.validateUrl(authenticatorConfig.getImageUrl());
}
Expand All @@ -240,7 +238,7 @@ public UserDefinedLocalAuthenticatorConfig updateUserDefinedLocalAuthenticator(
throw buildClientException(AuthenticatorMgtError.ERROR_NOT_FOUND_AUTHENTICATOR,
authenticatorConfig.getName());
}
authenticatorValidator.validateForBlank(DISPLAY_NAME, authenticatorConfig.getDisplayName());
authenticatorValidator.validateDisplayName(authenticatorConfig.getDisplayName());
if (authenticatorConfig.getImageUrl() != null) {
authenticatorValidator.validateUrl(authenticatorConfig.getImageUrl());
}
Expand Down Expand Up @@ -284,6 +282,40 @@ public UserDefinedLocalAuthenticatorConfig getUserDefinedLocalAuthenticator(Stri
authenticatorName, IdentityTenantUtil.getTenantId(tenantDomain));
}

/**
* Check whether any local or federated authenticator configuration exists with the given name.
*
* @param authenticatorName Name of the authenticator.
* @param tenantDomain Tenant domain.
* @return True if an authenticator with the given name exists.
* @throws AuthenticatorMgtException If an error occurs while checking the existence of the authenticator.
*/
public boolean isExistingAuthenticatorName(String authenticatorName, String tenantDomain)
throws AuthenticatorMgtException {

// Check whether an authenticator with the given name exists in the database.
if (dao.isExistingAuthenticatorName(authenticatorName, IdentityTenantUtil.getTenantId(tenantDomain))) {
return true;
}

/* Check whether an authenticator with the given name exists in the system defined authenticators
which are not saved in database. */
for (LocalAuthenticatorConfig localAuthenticator : localAuthenticators) {
if (localAuthenticator.getName().equals(authenticatorName)) {
return true;
}
}

/* Check whether an authenticator with the given name exists in the federated defined authenticators
which are not saved in database. */
for (FederatedAuthenticatorConfig federatedAuthenticator : federatedAuthenticators) {
if (federatedAuthenticator.getName().equals(authenticatorName)) {
return true;
}
}
return false;
}

private UserDefinedLocalAuthenticatorConfig resolveExistingAuthenticator(String authenticatorName,
String tenantDomain) throws AuthenticatorMgtException {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ public static class Query {
"WHERE NAME = :NAME; AND TENANT_ID = :TENANT_ID;";
public static final String GET_AUTHENTICATOR_SQL = "SELECT * FROM IDP_AUTHENTICATOR " +
"WHERE DEFINED_BY = :DEFINED_BY; AND NAME = :NAME; AND TENANT_ID = :TENANT_ID;";
public static final String IS_AUTHENTICATOR_EXISTS_BY_NAME_SQL = "SELECT ID FROM IDP_AUTHENTICATOR " +
"WHERE NAME = :NAME; AND TENANT_ID = :TENANT_ID;";
public static final String GET_ALL_USER_DEFINED_AUTHENTICATOR_SQL = "SELECT * FROM IDP_AUTHENTICATOR " +
"WHERE DEFINED_BY = :DEFINED_BY; AND TENANT_ID = :TENANT_ID;";
public static final String DELETE_AUTHENTICATOR_SQL = "DELETE FROM IDP_AUTHENTICATOR WHERE NAME = :NAME; " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,14 @@ List<UserDefinedLocalAuthenticatorConfig> getAllUserDefinedLocalAuthenticators(i
*/
void deleteUserDefinedLocalAuthenticator(String authenticatorConfigName, UserDefinedLocalAuthenticatorConfig
authenticatorConfig, int tenantId) throws AuthenticatorMgtException;

/**
* Check whether any local or federated authenticator configuration exists with the given name.
*
* @param authenticatorName Name of the authenticator.
* @param tenantId Tenant Id.
* @return True if an authenticator with the given name exists.
* @throws AuthenticatorMgtException If an error occurs while checking the existence of the authenticator.
*/
boolean isExistingAuthenticatorName(String authenticatorName, int tenantId) throws AuthenticatorMgtException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.wso2.carbon.identity.base.AuthenticatorPropertyConstants.DefinedByType;
import org.wso2.carbon.identity.core.util.IdentityDatabaseUtil;

import java.sql.ResultSet;
import java.util.ArrayList;
import java.util.List;

Expand Down Expand Up @@ -174,6 +175,25 @@ public void deleteUserDefinedLocalAuthenticator(String authenticatorConfigName,
}
}

public boolean isExistingAuthenticatorName(String authenticatorName, int tenantId)
throws AuthenticatorMgtException {

NamedJdbcTemplate jdbcTemplate = new NamedJdbcTemplate(IdentityDatabaseUtil.getDataSource());
try {
ResultSet results = jdbcTemplate.withTransaction(template ->
template.fetchSingleRecord(Query.IS_AUTHENTICATOR_EXISTS_BY_NAME_SQL,
(resultSet, rowNumber) -> resultSet,
statement -> {
statement.setString(Column.NAME, authenticatorName);
statement.setInt(Column.TENANT_ID, tenantId);
}));
return results != null;
} catch (TransactionException e) {
throw buildServerException(AuthenticatorMgtError.ERROR_WHILE_CHECKING_FOR_EXISTING_AUTHENTICATOR_BY_NAME, e,
authenticatorName);
}
}

private UserDefinedLocalAuthenticatorConfig getUserDefinedLocalAuthenticatorByName(String authenticatorConfigName,
int tenantId) throws TransactionException {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,20 @@ public void deleteUserDefinedLocalAuthenticator(String authenticatorConfigName,
}
}

@Override
public boolean isExistingAuthenticatorName(String authenticatorName, int tenantId)
throws AuthenticatorMgtException {

NamedJdbcTemplate jdbcTemplate = new NamedJdbcTemplate(IdentityDatabaseUtil.getDataSource());
try {
return jdbcTemplate.withTransaction(
template -> dao.isExistingAuthenticatorName(authenticatorName, tenantId));
} catch (TransactionException e) {
throw handleAuthenticatorMgtException(AuthenticatorMgtError
.ERROR_WHILE_CHECKING_FOR_EXISTING_AUTHENTICATOR_BY_NAME, e, authenticatorName);
}
}

/**
* Handle the authenticator management client exception.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,11 @@ public void deleteUserDefinedLocalAuthenticator(String authenticatorConfigName,
authenticatorMgtFacade.deleteUserDefinedLocalAuthenticator(authenticatorConfigName, authenticatorConfig,
tenantId);
}

@Override
public boolean isExistingAuthenticatorName(String authenticatorConfigName, int tenantId)
throws AuthenticatorMgtException {

return authenticatorMgtFacade.isExistingAuthenticatorName(authenticatorConfigName, tenantId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ public enum AuthenticatorMgtError {
"The authenticator already exists for the given name: %s."),
ERROR_INVALID_AUTHENTICATOR_NAME("60014", "Authenticator name is invalid.",
"The provided authenticator name %s is not in the expected format %s."),
ERROR_BLANK_FIELD_VALUE("60015", "Invalid empty or blank value.",
"Value for %s should not be empty or blank."),
ERROR_INVALID_URL("60017", "Invalid URL.",
ERROR_INVALID_DISPLAY_NAME("60015", "Authenticator display name is invalid.",
"The provided authenticator name %s is not in the expected format %s."),
ERROR_INVALID_URL("60016", "Invalid URL.",
"The provided url %s is not in the expected format %s."),

// Server errors.
Expand Down Expand Up @@ -121,7 +121,9 @@ public enum AuthenticatorMgtError {
ERROR_CODE_DELETING_ENDPOINT_CONFIG("65012", "Error while managing endpoint configurations.",
"Error while managing endpoint configurations for the user defined local authenticator %s."),
ERROR_CODE_HAVING_MULTIPLE_PROP("65013", "Multiple properties found", "Only actionId " +
"property is allowed for authenticator: %s.");
"property is allowed for authenticator: %s."),
ERROR_WHILE_CHECKING_FOR_EXISTING_AUTHENTICATOR_BY_NAME("65014", "Error while retrieving " +
"authenticator.", "Error while checking if an authenticator exists with the given name: %s.");

private final String code;
private final String message;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

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

import org.apache.commons.lang.StringUtils;
import org.wso2.carbon.identity.application.common.exception.AuthenticatorMgtClientException;
import org.wso2.carbon.identity.application.common.util.AuthenticatorMgtExceptionBuilder.AuthenticatorMgtError;

Expand All @@ -31,23 +30,27 @@
*/
public class UserDefinedLocalAuthenticatorValidator {

private static final String AUTHENTICATOR_NAME_REGEX = "^[a-zA-Z0-9][a-zA-Z0-9-_]*$";
private static final String AUTHENTICATOR_NAME_REGEX = "^custom-[a-zA-Z0-9-_]{3,}$";
private final Pattern authenticatorNameRegexPattern = Pattern.compile(AUTHENTICATOR_NAME_REGEX);

private static final String DISPLAY_NAME_REGEX = "^.{3,}$";
private final Pattern disaplayNameRegexPattern = Pattern.compile(DISPLAY_NAME_REGEX);

private static final String URL_REGEX = "^https?://.+";
private final Pattern urlRegexPattern = Pattern.compile(URL_REGEX);

/**
* Validate whether required fields exist.
* Validate the user defined local authenticator display name.
*
* @param fieldName Field name.
* @param fieldValue Field value.
* @throws AuthenticatorMgtClientException if the provided field is empty.
* @param displayName The display name.
* @throws AuthenticatorMgtClientException if the display name is not valid.
*/
public void validateForBlank(String fieldName, String fieldValue) throws AuthenticatorMgtClientException {
public void validateDisplayName(String displayName) throws AuthenticatorMgtClientException {

if (StringUtils.isBlank(fieldValue)) {
throw buildClientException(AuthenticatorMgtError.ERROR_BLANK_FIELD_VALUE, fieldName);
boolean isValidDisplayName = disaplayNameRegexPattern.matcher(displayName).matches();
if (!isValidDisplayName) {
throw buildClientException(AuthenticatorMgtError.ERROR_INVALID_DISPLAY_NAME,
displayName, DISPLAY_NAME_REGEX);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ public class ApplicationAuthenticatorServiceTest {
private static EndpointConfig endpointConfig;
private static EndpointConfig endpointConfigToBeUpdated;

private static final String AUTHENTICATOR1_NAME = "auth1";
private static final String AUTHENTICATOR2_NAME = "auth2";
private static final String AUTHENTICATOR_CONFIG_FOR_EXCEPTION_NAME = "exception_auth";
private static final String NON_EXIST_AUTHENTICATOR_NAME = "non_exist_auth";
private static final String AUTHENTICATOR1_NAME = "custom-auth1";
private static final String AUTHENTICATOR2_NAME = "custom-auth2";
private static final String AUTHENTICATOR_CONFIG_FOR_EXCEPTION_NAME = "custom-exception_auth";
private static final String NON_EXIST_AUTHENTICATOR_NAME = "custom-non_exist_auth";
private static final String SYSTEM_AUTHENTICATOR_NAME = "system_auth";

@BeforeClass
Expand Down Expand Up @@ -171,11 +171,11 @@ public void testCreateUserDefinedLocalAuthenticatorWithExistingAuthenticator(
}

@Test(priority = 3, expectedExceptions = AuthenticatorMgtException.class,
expectedExceptionsMessageRegExp = "Invalid empty or blank value.")
expectedExceptionsMessageRegExp = "Authenticator display name is invalid.")
public void testCreateUserDefinedLocalAuthenticatorWithBlankDisplayName() throws AuthenticatorMgtException {

UserDefinedLocalAuthenticatorConfig config = createUserDefinedAuthenticatorConfig("withBlankDisplayName",
AuthenticationType.IDENTIFICATION);
UserDefinedLocalAuthenticatorConfig config = createUserDefinedAuthenticatorConfig(
"custom-withBlankDisplayName", AuthenticationType.IDENTIFICATION);
Thisara-Welmilla marked this conversation as resolved.
Show resolved Hide resolved
config.setDisplayName("");
ApplicationCommonServiceDataHolder.getInstance().getApplicationAuthenticatorService()
.addUserDefinedLocalAuthenticator(config, tenantDomain);
Expand Down Expand Up @@ -351,6 +351,22 @@ public void testGetUserDefinedLocalAuthenticator(UserDefinedLocalAuthenticatorCo
}

@Test(priority = 16)
public void testIsExistingAuthenticatorName() throws AuthenticatorMgtException {

Assert.assertTrue(ApplicationCommonServiceDataHolder.getInstance().
getApplicationAuthenticatorService().isExistingAuthenticatorName(
authenticatorConfig1.getName(), tenantDomain));
}

@Test(priority = 17)
public void testIsExistingAuthenticatorNameForNonExistName() throws AuthenticatorMgtException {

Assert.assertFalse(ApplicationCommonServiceDataHolder.getInstance().
getApplicationAuthenticatorService().isExistingAuthenticatorName(
NON_EXIST_AUTHENTICATOR_NAME, tenantDomain));
}

@Test(priority = 18)
public void testDeleteUserDefinedLocalAuthenticatorWithActionException() throws Exception {

ActionManagementService actionManagementServiceForException = mock(ActionManagementService.class);
Expand All @@ -368,7 +384,7 @@ public void testDeleteUserDefinedLocalAuthenticatorWithActionException() throws
authenticatorConfigForException.getName(), tenantDomain));
}

@Test(priority = 17, dataProvider = "authenticatorConfigToModify")
@Test(priority = 19, dataProvider = "authenticatorConfigToModify")
public void testDeleteUserDefinedLocalAuthenticator(UserDefinedLocalAuthenticatorConfig config)
throws AuthenticatorMgtException {

Expand All @@ -378,7 +394,7 @@ public void testDeleteUserDefinedLocalAuthenticator(UserDefinedLocalAuthenticato
.getLocalAuthenticatorByName(config.getName()));
}

@Test(priority = 18)
@Test(priority = 20)
public void testDeleteUserDefinedLocalAuthenticatorWithNonExistingAuthenticator() throws AuthenticatorMgtException {

// Assert that no exception is thrown when trying to delete a non-existing authenticator.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ public class AuthenticatorManagementDAOImplTest {
private UserDefinedLocalAuthenticatorConfig authenticatorForUpdate;
private UserDefinedLocalAuthenticatorConfig authenticatorForUpdateForException;

private static final String AUTHENTICATOR1_NAME = "auth1";
private static final String AUTHENTICATOR2_NAME = "auth2";
private static final String AUTHENTICATOR1_NAME = "custom-auth1";
private static final String AUTHENTICATOR2_NAME = "custom-auth2";
private static final String AUTHENTICATOR_CONFIG_FOR_EXCEPTION_NAME = "exception_auth";
private static final String NON_EXIST_AUTHENTICATOR_NAME = "non_exist_auth";

Expand Down Expand Up @@ -172,7 +172,21 @@ public void testGetUserDefinedLocalAuthenticatorForNonExist() throws Authenticat
NON_EXIST_AUTHENTICATOR_NAME, tenantId));
}

@Test(dataProvider = "authenticatorConfig", priority = 9)
@Test(priority = 9)
public void testIsExistingAuthenticatorName() throws AuthenticatorMgtException {

Assert.assertTrue(authenticatorManagementDAO.isExistingAuthenticatorName(
authenticatorConfig1.getName(), tenantId));
}

@Test(priority = 10)
public void testIsExistingAuthenticatorNameForNonExistName() throws AuthenticatorMgtException {

Assert.assertFalse(authenticatorManagementDAO.isExistingAuthenticatorName(
NON_EXIST_AUTHENTICATOR_NAME, tenantId));
}

@Test(dataProvider = "authenticatorConfig", priority = 11)
public void testDeleteUserDefinedLocalAuthenticator(UserDefinedLocalAuthenticatorConfig config)
throws AuthenticatorMgtException {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.apache.commons.logging.LogFactory;
import org.wso2.carbon.identity.application.common.ApplicationAuthenticatorService;
import org.wso2.carbon.identity.application.common.ProvisioningConnectorService;
import org.wso2.carbon.identity.application.common.exception.AuthenticatorMgtException;
import org.wso2.carbon.identity.application.common.model.ClaimConfig;
import org.wso2.carbon.identity.application.common.model.ClaimMapping;
import org.wso2.carbon.identity.application.common.model.FederatedAuthenticatorConfig;
Expand Down Expand Up @@ -2234,7 +2235,7 @@ private void validateFederatedAuthenticatorConfigName(FederatedAuthenticatorConf
}
} else {
// Check if the given authenticator name is already taken.
if (getFederatedAuthenticatorByName(config.getName(), tenantDomain) != null) {
if (isExistingAuthentication(config.getName(), tenantDomain)) {
throw IdPManagementUtil.handleClientException(IdPManagementConstants.ErrorMessage
.ERROR_CODE_AUTHENTICATOR_NAME_ALREADY_TAKEN, config.getName());
}
Expand Down Expand Up @@ -2389,13 +2390,16 @@ public FederatedAuthenticatorConfig[] getAllFederatedAuthenticators(String tenan
return allFederatedAuthenticators.toArray(new FederatedAuthenticatorConfig[0]);
}

private FederatedAuthenticatorConfig getFederatedAuthenticatorByName(String authenticatorName, String tenantDomain)
private boolean isExistingAuthentication(String authenticatorName, String tenantDomain)
throws IdentityProviderManagementException {

return Arrays.stream(getAllFederatedAuthenticators(tenantDomain))
.filter(authenticator -> authenticator.getName().equals(authenticatorName))
.findFirst()
.orElse(null);
try {
return ApplicationAuthenticatorService.getInstance()
.isExistingAuthenticatorName(authenticatorName, tenantDomain);
} catch (AuthenticatorMgtException e) {
throw IdPManagementUtil.handleClientException(
IdPManagementConstants.ErrorMessage.ERROR_CODE_AUTHENTICATOR_NAME_ALREADY_TAKEN, authenticatorName);
}
}

/**
Expand Down
Loading
Loading