This is an automated email from the ASF dual-hosted git repository. vorburger pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/fineract.git
commit 753554e2819f23fbfe2918dc75acb1d4bc95df6c Author: Michael Vorburger <[email protected]> AuthorDate: Sun Mar 8 18:17:16 2020 +0100 Fix audit trails filter (FINERACT-808) As per the great analysis by Manthan Surkar (@thesmallstar) in the original PR https://github.com/apache/fineract/pull/723, the cause of the FINERACT-808 bug was that "the backend would treat "UPDATE" and similar strings as SQL injection". The root cause of that was that (IMHO..) how Fineract does SQL injection is more of a workaround (blacklisting some keywords and some heuristic checks) then how this really should be done (by using JDBC Prepared statements with arguments for all external data, instead inlined SQL). This also lays the foundation for more like this in FINERACT-854. --- .../fineract/commands/api/AuditsApiResource.java | 81 ++++--------- .../commands/api/MakercheckersApiResource.java | 61 +++------- .../commands/service/AuditReadPlatformService.java | 7 +- .../service/AuditReadPlatformServiceImpl.java | 59 +++------- .../infrastructure/security/utils/SQLBuilder.java | 126 +++++++++++++++++++++ .../security/utils/SQLBuilderTest.java | 86 ++++++++++++++ 6 files changed, 269 insertions(+), 151 deletions(-) diff --git a/fineract-provider/src/main/java/org/apache/fineract/commands/api/AuditsApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/commands/api/AuditsApiResource.java index 79d1b81..b1f7f4e 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/commands/api/AuditsApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/commands/api/AuditsApiResource.java @@ -38,17 +38,16 @@ import javax.ws.rs.QueryParam; import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.UriInfo; -import org.apache.commons.lang.StringUtils; import org.apache.fineract.commands.data.AuditData; import org.apache.fineract.commands.data.AuditSearchData; import org.apache.fineract.commands.service.AuditReadPlatformService; -import org.apache.fineract.infrastructure.core.api.ApiParameterHelper; import org.apache.fineract.infrastructure.core.api.ApiRequestParameterHelper; import org.apache.fineract.infrastructure.core.data.PaginationParameters; import org.apache.fineract.infrastructure.core.serialization.ApiRequestJsonSerializationSettings; import org.apache.fineract.infrastructure.core.serialization.DefaultToApiJsonSerializer; import org.apache.fineract.infrastructure.core.service.Page; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; +import org.apache.fineract.infrastructure.security.utils.SQLBuilder; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Component; @@ -104,7 +103,7 @@ public class AuditsApiResource { this.context.authenticatedUser().validateHasReadPermission(this.resourceNameForPermissions); final PaginationParameters parameters = PaginationParameters.instance(paged, offset, limit, orderBy, sortOrder); - final String extraCriteria = getExtraCriteria(actionName, entityName, resourceId, makerId, makerDateTimeFrom, makerDateTimeTo, + final SQLBuilder extraCriteria = getExtraCriteria(actionName, entityName, resourceId, makerId, makerDateTimeFrom, makerDateTimeTo, checkerId, checkerDateTimeFrom, checkerDateTimeTo, processingResult, officeId, groupId, clientId, loanId, savingsAccountId); final ApiRequestJsonSerializationSettings settings = this.apiRequestParameterHelper.process(uriInfo.getQueryParameters()); @@ -157,69 +156,29 @@ public class AuditsApiResource { return this.toApiJsonSerializerSearchTemplate.serialize(settings, auditSearchData, RESPONSE_DATA_PARAMETERS_SEARCH_TEMPLATE); } - private String getExtraCriteria(final String actionName, final String entityName, final Long resourceId, final Long makerId, + private SQLBuilder getExtraCriteria(final String actionName, final String entityName, final Long resourceId, final Long makerId, final String makerDateTimeFrom, final String makerDateTimeTo, final Long checkerId, final String checkerDateTimeFrom, final String checkerDateTimeTo, final Integer processingResult, final Integer officeId, final Integer groupId, final Integer clientId, final Integer loanId, final Integer savingsAccountId) { - String extraCriteria = ""; - - if (actionName != null) { - extraCriteria += " and aud.action_name = " + ApiParameterHelper.sqlEncodeString(actionName); - } + SQLBuilder extraCriteria = new SQLBuilder(); + extraCriteria.addNonNullCriteria("aud.action_name = ", actionName); if (entityName != null) { - extraCriteria += " and aud.entity_name like " + ApiParameterHelper.sqlEncodeString(entityName + "%"); - } - - if (resourceId != null) { - extraCriteria += " and aud.resource_id = " + resourceId; - } - if (makerId != null) { - extraCriteria += " and aud.maker_id = " + makerId; - } - if (checkerId != null) { - extraCriteria += " and aud.checker_id = " + checkerId; - } - if (makerDateTimeFrom != null) { - extraCriteria += " and aud.made_on_date >= " + ApiParameterHelper.sqlEncodeString(makerDateTimeFrom); - } - if (makerDateTimeTo != null) { - extraCriteria += " and aud.made_on_date <= " + ApiParameterHelper.sqlEncodeString(makerDateTimeTo); - } - if (checkerDateTimeFrom != null) { - extraCriteria += " and aud.checked_on_date >= " + ApiParameterHelper.sqlEncodeString(checkerDateTimeFrom); - } - if (checkerDateTimeTo != null) { - extraCriteria += " and aud.checked_on_date <= " + ApiParameterHelper.sqlEncodeString(checkerDateTimeTo); - } - - if (processingResult != null) { - extraCriteria += " and aud.processing_result_enum = " + processingResult; - } - - if (officeId != null) { - extraCriteria += " and aud.office_id = " + officeId; - } - - if (groupId != null) { - extraCriteria += " and aud.group_id = " + groupId; - } - - if (clientId != null) { - extraCriteria += " and aud.client_id = " + clientId; - } - - if (loanId != null) { - extraCriteria += " and aud.loan_id = " + loanId; - } - - if (savingsAccountId != null) { - extraCriteria += " and aud.savings_account_id = " + savingsAccountId; - } - - if (StringUtils.isNotBlank(extraCriteria)) { - extraCriteria = extraCriteria.substring(4); - } + extraCriteria.addCriteria("aud.entity_name like", entityName + "%"); + } + extraCriteria.addNonNullCriteria("aud.resource_id = ", resourceId); + extraCriteria.addNonNullCriteria("aud.maker_id = ", makerId); + extraCriteria.addNonNullCriteria("aud.checker_id = ", checkerId); + extraCriteria.addNonNullCriteria("aud.made_on_date >= ", makerDateTimeFrom); + extraCriteria.addNonNullCriteria("aud.made_on_date <= ", makerDateTimeTo); + extraCriteria.addNonNullCriteria("aud.checked_on_date >= ", checkerDateTimeFrom); + extraCriteria.addNonNullCriteria("aud.checked_on_date <= ", checkerDateTimeTo); + extraCriteria.addNonNullCriteria("aud.processing_result_enum = ", processingResult); + extraCriteria.addNonNullCriteria("aud.office_id = ", officeId); + extraCriteria.addNonNullCriteria("aud.group_id = ", groupId); + extraCriteria.addNonNullCriteria("aud.client_id = ", clientId); + extraCriteria.addNonNullCriteria("aud.loan_id = ", loanId); + extraCriteria.addNonNullCriteria("aud.savings_account_id = ", savingsAccountId); return extraCriteria; } diff --git a/fineract-provider/src/main/java/org/apache/fineract/commands/api/MakercheckersApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/commands/api/MakercheckersApiResource.java index c714722..efe7547 100755 --- a/fineract-provider/src/main/java/org/apache/fineract/commands/api/MakercheckersApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/commands/api/MakercheckersApiResource.java @@ -45,12 +45,12 @@ import org.apache.fineract.commands.data.AuditData; import org.apache.fineract.commands.data.AuditSearchData; import org.apache.fineract.commands.service.AuditReadPlatformService; import org.apache.fineract.commands.service.PortfolioCommandSourceWritePlatformService; -import org.apache.fineract.infrastructure.core.api.ApiParameterHelper; import org.apache.fineract.infrastructure.core.api.ApiRequestParameterHelper; import org.apache.fineract.infrastructure.core.data.CommandProcessingResult; import org.apache.fineract.infrastructure.core.exception.UnrecognizedQueryParamException; import org.apache.fineract.infrastructure.core.serialization.ApiRequestJsonSerializationSettings; import org.apache.fineract.infrastructure.core.serialization.DefaultToApiJsonSerializer; +import org.apache.fineract.infrastructure.security.utils.SQLBuilder; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Component; @@ -98,7 +98,7 @@ public class MakercheckersApiResource { @QueryParam("groupId") @ApiParam(value = "groupId") final Integer groupId, @QueryParam("clientId") @ApiParam(value = "clientId") final Integer clientId, @QueryParam("loanid") @ApiParam(value = "loanid") final Integer loanId, @QueryParam("savingsAccountId") @ApiParam(value = "savingsAccountId") final Integer savingsAccountId) { - final String extraCriteria = getExtraCriteria(actionName, entityName, resourceId, makerId, makerDateTimeFrom, makerDateTimeTo, + final SQLBuilder extraCriteria = getExtraCriteria(actionName, entityName, resourceId, makerId, makerDateTimeFrom, makerDateTimeTo, officeId, groupId, clientId, loanId, savingsAccountId); final ApiRequestJsonSerializationSettings settings = this.apiRequestParameterHelper.process(uriInfo.getQueryParameters()); @@ -164,55 +164,24 @@ public class MakercheckersApiResource { return this.toApiJsonSerializerAudit.serialize(CommandProcessingResult.commandOnlyResult(id)); } - private String getExtraCriteria(final String actionName, final String entityName, final Long resourceId, final Long makerId, + private SQLBuilder getExtraCriteria(final String actionName, final String entityName, final Long resourceId, final Long makerId, final String makerDateTimeFrom, final String makerDateTimeTo, final Integer officeId, final Integer groupId, final Integer clientId, final Integer loanId, final Integer savingsAccountId) { - String extraCriteria = ""; - - if (actionName != null) { - extraCriteria += " and aud.action_name = " + ApiParameterHelper.sqlEncodeString(actionName); - } + SQLBuilder extraCriteria = new SQLBuilder(); + extraCriteria.addNonNullCriteria("aud.action_name = ", actionName); if (entityName != null) { - extraCriteria += " and aud.entity_name like " + ApiParameterHelper.sqlEncodeString(entityName + "%"); - } - - if (resourceId != null) { - extraCriteria += " and aud.resource_id = " + resourceId; - } - if (makerId != null) { - extraCriteria += " and aud.maker_id = " + makerId; - } - if (makerDateTimeFrom != null) { - extraCriteria += " and aud.made_on_date >= " + ApiParameterHelper.sqlEncodeString(makerDateTimeFrom); - } - if (makerDateTimeTo != null) { - extraCriteria += " and aud.made_on_date <= " + ApiParameterHelper.sqlEncodeString(makerDateTimeTo); - } - - if (officeId != null) { - extraCriteria += " and aud.office_id = " + officeId; - } - - if (groupId != null) { - extraCriteria += " and aud.group_id = " + groupId; - } - - if (clientId != null) { - extraCriteria += " and aud.client_id = " + clientId; - } - - if (loanId != null) { - extraCriteria += " and aud.loan_id = " + loanId; - } - - if (savingsAccountId != null) { - extraCriteria += " and aud.savings_account_id = " + savingsAccountId; - } - - if (StringUtils.isNotBlank(extraCriteria)) { - extraCriteria = extraCriteria.substring(4); + extraCriteria.addCriteria("aud.entity_name like ", entityName + "%"); } + extraCriteria.addNonNullCriteria("aud.resource_id = ", resourceId); + extraCriteria.addNonNullCriteria("aud.maker_id = ", makerId); + extraCriteria.addNonNullCriteria("aud.made_on_date >= ", makerDateTimeFrom); + extraCriteria.addNonNullCriteria("aud.made_on_date <= ", makerDateTimeTo); + extraCriteria.addNonNullCriteria("aud.office_id = ", officeId); + extraCriteria.addNonNullCriteria("aud.group_id = ", groupId); + extraCriteria.addNonNullCriteria("aud.client_id = ", clientId); + extraCriteria.addNonNullCriteria("aud.loan_id = ", loanId); + extraCriteria.addNonNullCriteria("aud.savings_account_id = ", savingsAccountId); return extraCriteria; } diff --git a/fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformService.java b/fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformService.java index ef71553..c3d71c5 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformService.java +++ b/fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformService.java @@ -23,14 +23,15 @@ import org.apache.fineract.commands.data.AuditData; import org.apache.fineract.commands.data.AuditSearchData; import org.apache.fineract.infrastructure.core.data.PaginationParameters; import org.apache.fineract.infrastructure.core.service.Page; +import org.apache.fineract.infrastructure.security.utils.SQLBuilder; public interface AuditReadPlatformService { - Collection<AuditData> retrieveAuditEntries(String extraCriteria, boolean includeJson); + Collection<AuditData> retrieveAuditEntries(SQLBuilder extraCriteria, boolean includeJson); - Page<AuditData> retrievePaginatedAuditEntries(String extraCriteria, boolean includeJson, PaginationParameters parameters); + Page<AuditData> retrievePaginatedAuditEntries(SQLBuilder extraCriteria, boolean includeJson, PaginationParameters parameters); - Collection<AuditData> retrieveAllEntriesToBeChecked(String extraCriteria, boolean includeJson); + Collection<AuditData> retrieveAllEntriesToBeChecked(SQLBuilder extraCriteria, boolean includeJson); AuditData retrieveAuditEntry(Long auditId); diff --git a/fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformServiceImpl.java b/fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformServiceImpl.java index 6988ba5..41b48ab 100755 --- a/fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformServiceImpl.java +++ b/fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformServiceImpl.java @@ -44,6 +44,7 @@ import org.apache.fineract.infrastructure.core.service.PaginationHelper; import org.apache.fineract.infrastructure.core.service.RoutingDataSource; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; import org.apache.fineract.infrastructure.security.utils.ColumnValidator; +import org.apache.fineract.infrastructure.security.utils.SQLBuilder; import org.apache.fineract.organisation.office.data.OfficeData; import org.apache.fineract.organisation.office.service.OfficeReadPlatformService; import org.apache.fineract.organisation.staff.data.StaffData; @@ -184,36 +185,23 @@ public class AuditReadPlatformServiceImpl implements AuditReadPlatformService { } @Override - public Collection<AuditData> retrieveAuditEntries(final String extraCriteria, final boolean includeJson) { - - String updatedExtraCriteria = ""; - if (StringUtils.isNotBlank(extraCriteria)) { - updatedExtraCriteria = " where (" + extraCriteria + ")"; - } - - updatedExtraCriteria += " order by aud.id DESC limit " + PaginationParameters.getCheckedLimit(null); - return retrieveEntries("audit", updatedExtraCriteria, includeJson, StringUtils.isNotBlank(extraCriteria)); + public Collection<AuditData> retrieveAuditEntries(final SQLBuilder extraCriteria, final boolean includeJson) { + return retrieveEntries("audit", extraCriteria, " order by aud.id DESC limit " + PaginationParameters.getCheckedLimit(null), includeJson); } @Override - public Page<AuditData> retrievePaginatedAuditEntries(final String extraCriteria, final boolean includeJson, + public Page<AuditData> retrievePaginatedAuditEntries(final SQLBuilder extraCriteria, final boolean includeJson, final PaginationParameters parameters) { this.paginationParametersDataValidator.validateParameterValues(parameters, supportedOrderByValues, "audits"); final AppUser currentUser = this.context.authenticatedUser(); final String hierarchy = currentUser.getOffice().getHierarchy(); - String updatedExtraCriteria = ""; - if (StringUtils.isNotBlank(extraCriteria)) { - updatedExtraCriteria = " where (" + extraCriteria + ")"; - } - final AuditMapper rm = new AuditMapper(); final StringBuilder sqlBuilder = new StringBuilder(200); sqlBuilder.append("select SQL_CALC_FOUND_ROWS "); sqlBuilder.append(rm.schema(includeJson, hierarchy)); - sqlBuilder.append(' ').append(updatedExtraCriteria); - this.columnValidator.validateSqlInjection(sqlBuilder.toString(), extraCriteria); + sqlBuilder.append(' ').append(extraCriteria.getSQLTemplate()); if (parameters.isOrderByRequested()) { sqlBuilder.append(' ').append(parameters.orderBySql()); this.columnValidator.validateSqlInjection(sqlBuilder.toString(), parameters.orderBySql()); @@ -229,27 +217,18 @@ public class AuditReadPlatformServiceImpl implements AuditReadPlatformService { logger.info("sql: " + sqlBuilder.toString()); final String sqlCountRows = "SELECT FOUND_ROWS()"; - return this.paginationHelper.fetchPage(this.jdbcTemplate, sqlCountRows, sqlBuilder.toString(), new Object[] {}, rm); + return this.paginationHelper.fetchPage(this.jdbcTemplate, sqlCountRows, sqlBuilder.toString(), extraCriteria.getArguments(), rm); } @Override - public Collection<AuditData> retrieveAllEntriesToBeChecked(final String extraCriteria, final boolean includeJson) { - - String updatedExtraCriteria = ""; - if (StringUtils.isNotBlank(extraCriteria)) { - updatedExtraCriteria = " where (" + extraCriteria + ")" + " and aud.processing_result_enum = 2"; - } else { - updatedExtraCriteria = " where aud.processing_result_enum = 2"; - } - - updatedExtraCriteria += " group by aud.id order by aud.id"; - - return retrieveEntries("makerchecker", updatedExtraCriteria, includeJson, StringUtils.isNotBlank(extraCriteria)); + public Collection<AuditData> retrieveAllEntriesToBeChecked(final SQLBuilder extraCriteria, final boolean includeJson) { + extraCriteria.addCriteria("aud.processing_result_enum = ", 2); + return retrieveEntries("makerchecker", extraCriteria, " group by aud.id order by aud.id", includeJson); } - public Collection<AuditData> retrieveEntries(final String useType, final String extraCriteria, final boolean includeJson, boolean isExtraCritereaIncluded) { + private Collection<AuditData> retrieveEntries(final String useType, final SQLBuilder extraCriteria, final String groupAndOrderBySQL, final boolean includeJson) { - if (!(useType.equals("audit") || useType.equals("makerchecker"))) { throw new PlatformDataIntegrityException( + if ((!useType.equals("audit") && !useType.equals("makerchecker"))) { throw new PlatformDataIntegrityException( "error.msg.invalid.auditSearchTemplate.useType", "Invalid Audit Search Template UseType: " + useType); } final AppUser currentUser = this.context.authenticatedUser(); @@ -270,13 +249,11 @@ public class AuditReadPlatformServiceImpl implements AuditReadPlatformService { + " join m_role_permission rp on rp.permission_id = p.id" + " join m_role r on r.id = rp.role_id " + " join m_appuser_role ur on ur.role_id = r.id and ur.appuser_id = " + currentUser.getId(); } - sql += extraCriteria; - if(isExtraCritereaIncluded){ - this.columnValidator.validateSqlInjection(sql, extraCriteria); - } + sql += extraCriteria.getSQLTemplate(); + sql += groupAndOrderBySQL; logger.info("sql: " + sql); - return this.jdbcTemplate.query(sql, rm, new Object[] {}); + return this.jdbcTemplate.query(sql, rm, extraCriteria.getArguments()); } @Override @@ -289,7 +266,7 @@ public class AuditReadPlatformServiceImpl implements AuditReadPlatformService { final String sql = "select " + rm.schema(true, hierarchy) + " where aud.id = ? "; - final AuditData auditResult = this.jdbcTemplate.queryForObject(sql, rm, new Object[] {auditId}); + final AuditData auditResult = this.jdbcTemplate.queryForObject(sql, rm, auditId); return replaceIdsOnAuditData(auditResult); } @@ -461,18 +438,18 @@ public class AuditReadPlatformServiceImpl implements AuditReadPlatformService { sql += makercheckerCapabilityOnly(useType, currentUser); sql += " order by if(action_name in ('CREATE', 'DELETE', 'UPDATE'), action_name, 'ZZZ'), action_name"; final ActionNamesMapper mapper = new ActionNamesMapper(); - final List<String> actionNames = this.jdbcTemplate.query(sql, mapper, new Object[] {}); + final List<String> actionNames = this.jdbcTemplate.query(sql, mapper); sql = " select distinct(entity_name) as entityName from m_permission p "; sql += makercheckerCapabilityOnly(useType, currentUser); sql += " order by if(grouping = 'datatable', 'ZZZ', entity_name), entity_name"; final EntityNamesMapper mapper2 = new EntityNamesMapper(); - final List<String> entityNames = this.jdbcTemplate.query(sql, mapper2, new Object[] {}); + final List<String> entityNames = this.jdbcTemplate.query(sql, mapper2); Collection<ProcessingResultLookup> processingResults = null; if (useType.equals("audit")) { final ProcessingResultsMapper mapper3 = new ProcessingResultsMapper(); - processingResults = this.jdbcTemplate.query(mapper3.schema(), mapper3, new Object[] {}); + processingResults = this.jdbcTemplate.query(mapper3.schema(), mapper3); } return new AuditSearchData(appUsers, actionNames, entityNames, processingResults); diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/SQLBuilder.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/SQLBuilder.java new file mode 100644 index 0000000..98f4735 --- /dev/null +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/SQLBuilder.java @@ -0,0 +1,126 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.security.utils; + +import java.sql.PreparedStatement; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.regex.Pattern; + +/** + * Utility to assemble the WHERE clause of an SQL query without the risk of SQL injection. + * + * <p>When using this utility instead of manually assembling SQL queries, then + * {@link SQLInjectionValidator} should not be required anymore. (Correctly using + * this means only ever passing completely fixed String literals to .) + * + * @author Michael Vorburger <[email protected]> + */ +public class SQLBuilder { + + private final static Pattern ATOZ = Pattern.compile("[a-z]+"); + + // This holds the query string, with the '?' placeholders, but no argument values + private final StringBuilder sb = new StringBuilder(); + + // This holds the arguments, in the order of the '?' placeholders in sb + private final List<Object> args = new ArrayList<>(); + + /** + * Adds a criteria for a SQL WHERE clause. + * All criteria are appended by AND (support for OR, or nesting, can be added when needed). + * @param criteria The name of the column to be filtered, and an operator; e.g. "name =" or "age >" (but without '?' placeholder) + * @param argument The argument to be filtered on (e.g. "Michael" or 123). The null value is explicitly permitted. + */ + public void addCriteria(String criteria, Object argument) { + if (criteria == null || criteria.trim().isEmpty()) { + throw new IllegalArgumentException("criteria cannot be null"); + } + String trimmedCriteria = criteria.trim(); + if (trimmedCriteria.isEmpty()) { + throw new IllegalArgumentException("criteria cannot be null"); + } + if (trimmedCriteria.contains("?")) { + throw new IllegalArgumentException("criteria cannot contain a '?' (that is automatically added at the end): " + trimmedCriteria); + } + int columnOperatorIndex = trimmedCriteria.indexOf(' '); + if (columnOperatorIndex == -1) { + throw new IllegalArgumentException("criteria missing operator: " + trimmedCriteria); + } + String columnName = trimmedCriteria.substring(0, columnOperatorIndex).trim().toLowerCase(Locale.ROOT); + if (!ATOZ.matcher(columnName).matches()) { + throw new IllegalArgumentException("criteria column name must match [a-z]: " + trimmedCriteria); + } + String operator = trimmedCriteria.substring(columnOperatorIndex).trim(); + if (operator.indexOf(' ') > -1) { + throw new IllegalArgumentException("criteria cannot contain more than 1 space (between column name and operator): " + trimmedCriteria); + } + if (!operator.equals("=") && !operator.equals("<") && !operator.equals(">") + && !operator.equals("<=") && !operator.equals(">=") && !operator.equals("<>") + && !operator.equals("LIKE") && !operator.equals("like")) { + // add support for SQL's BETWEEN and IN, if/when ever needed.. (it's a little more than just adding above, as it can have multiple arguments) + throw new IllegalArgumentException("criteria must end with valid SQL operator for WHERE: " + trimmedCriteria); + } + + if (sb.length() > 0) { + sb.append(" AND "); + } + sb.append(trimmedCriteria); + sb.append(" ?"); + args.add(argument); + } + + /** + * Delegates to {@link #addCriteria(String, Object)} if argument is not null, otherwise does nothing. + */ + public void addNonNullCriteria(String criteria, Object argument) { + if (argument != null) { + addCriteria(criteria, argument); + } + } + + /** + * Returns a SQL WHERE clause, created from the {@link #addCriteria(String, Object)}, with '?' placeholders. + * @return SQL WHERE clause, almost always starting with " WHERE ..." (unless no criteria, then empty) + */ + public String getSQLTemplate() { + if (sb.length() > 0) { + return " WHERE " + sb.toString(); + } + return ""; + } + + /** + * Returns the arguments for the WHERE clause. + * @return Object array suitable for use with Spring Framework JdbcTemplate (or plain JDBC {@link PreparedStatement}) + */ + public Object[] getArguments() { + return args.toArray(); + } + + /* + * Returns a String representation suitable for debugging and log output. + * This is ONLY intended for debugging in logs, and NEVER for passing to a JDBC database. + @Override + public String toString() { + return "SQLBuilder{..."; // TODO implement this... + } + */ +} diff --git a/fineract-provider/src/test/java/org/apache/fineract/infrastructure/security/utils/SQLBuilderTest.java b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/security/utils/SQLBuilderTest.java new file mode 100644 index 0000000..34358a6 --- /dev/null +++ b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/security/utils/SQLBuilderTest.java @@ -0,0 +1,86 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.security.utils; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; + +import org.junit.Test; + +/** + * Unit Test for {@link SQLBuilder}. + * + * @author Michael Vorburger <[email protected]> + */ +public class SQLBuilderTest { + + @Test + public void testEmpty() { + SQLBuilder sqlBuilder = new SQLBuilder(); + assertEquals("", sqlBuilder.getSQLTemplate()); + assertArrayEquals(new Object[] {}, sqlBuilder.getArguments()); + // TODO assertEquals("SQLBuilder{}", sqlBuilder.toString()); + } + + @Test + public void testUsage() { + SQLBuilder sqlBuilder = new SQLBuilder(); + sqlBuilder.addCriteria("name =", "Michael"); + sqlBuilder.addCriteria("hobby LIKE ", "Mifos/Apache Fineract"); + sqlBuilder.addCriteria("age < ", 123); + assertEquals(" WHERE name = ? AND hobby LIKE ? AND age < ?", sqlBuilder.getSQLTemplate()); + assertArrayEquals(new Object[] { "Michael", "Mifos/Apache Fineract", 123}, sqlBuilder.getArguments()); + // TODO assertEquals("SQLBuilder{ WHERE name = ['Michael'] AND hobby = ['Mifos/Apache Fineract'] AND age < [123]}", sqlBuilder.toString()); + } + + @Test + public void testNullArgument() { + SQLBuilder sqlBuilder = new SQLBuilder(); + sqlBuilder.addCriteria("ref =", null); + assertEquals(" WHERE ref = ?", sqlBuilder.getSQLTemplate()); + assertArrayEquals(new Object[] { null }, sqlBuilder.getArguments()); + // TODO assertEquals("SQLBuilder{ WHERE ref = [null] }", sqlBuilder.toString()); + } + + @Test + public void testLowerAndUpperCaseOperators() { + SQLBuilder sqlBuilder = new SQLBuilder(); + sqlBuilder.addCriteria("hobby LIKE ", "Mifos/Apache Fineract"); + sqlBuilder.addCriteria("hobby like ", "Mifos/Apache Fineract"); + } + + @Test + public void testAddIllegalArguments() { + assertThrows("space between column and operator", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("age<", 123)); + assertThrows("null Criteria Fragment", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria(null, "argument")); + assertThrows("empty Criteria Fragment", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("", "argument")); + assertThrows("space only Criteria Fragment", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria(" ", "argument")); + assertThrows("Criteria Fragment with ?", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("age = ?", 123)); + assertThrows("Criteria Fragment missing operator", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("age", 123)); + assertThrows("Criteria starts with AND", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("and age = ?", 123)); + assertThrows("Criteria ends with AND", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("age = ? and", 123)); + assertThrows("Criteria starts with OR", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("or age =", 123)); + assertThrows("Criteria ends with OR", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("age = ? or", 123)); + assertThrows("Criteria contains opening parentheis", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("(age =", 123)); + assertThrows("Criteria contains closing parentheis", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("age = ?)", 123)); + assertThrows("Offset corner case", IllegalArgumentException.class, () -> new SQLBuilder().addCriteria("age< = ?)", 123)); + + } +}
