From 7950ca3e58d10f1255701824fcc9ab505486a24c Mon Sep 17 00:00:00 2001 From: Manthan Surkar Date: Wed, 26 Aug 2020 04:24:23 +0530 Subject: [PATCH] FINERACT-854 Removed string concatenated SQL in GroupReadPlatform --- .../core/service/SearchParameters.java | 38 +++++++-- .../security/utils/SQLBuilder.java | 2 +- .../group/api/CentersApiResource.java | 4 +- .../group/api/GroupsApiResource.java | 5 +- .../service/GroupReadPlatformServiceImpl.java | 83 +++++-------------- 5 files changed, 60 insertions(+), 72 deletions(-) diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/SearchParameters.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/SearchParameters.java index 11e8af51b..9a4d99532 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/SearchParameters.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/SearchParameters.java @@ -77,9 +77,9 @@ public final class SearchParameters { maxLimitAllowed, orderBy, sortOrder, staffId, accountNo, loanId, savingsId, orphansOnly, isSelfUser); } - public static SearchParameters forGroups(final String sqlSearch, final Long officeId, final Long staffId, final String externalId, - final String name, final String hierarchy, final Integer offset, final Integer limit, final String orderBy, - final String sortOrder, final Boolean orphansOnly) { + public static SearchParameters forGroups(final Long officeId, final Long staffId, final String externalId, final String name, + final String hierarchy, final Integer offset, final Integer limit, final String orderBy, final String sortOrder, + final Boolean orphansOnly) { final Integer maxLimitAllowed = getCheckedLimit(limit); final String accountNo = null; @@ -87,8 +87,8 @@ public final class SearchParameters { final Long savingsId = null; final boolean isSelfUser = false; - return new SearchParameters(sqlSearch, officeId, externalId, name, hierarchy, null, null, offset, maxLimitAllowed, orderBy, - sortOrder, staffId, accountNo, loanId, savingsId, orphansOnly, isSelfUser); + return new SearchParameters(officeId, externalId, name, hierarchy, null, null, offset, maxLimitAllowed, orderBy, sortOrder, staffId, + accountNo, loanId, savingsId, orphansOnly, isSelfUser); } public static SearchParameters forOffices(final String orderBy, final String sortOrder) { @@ -301,6 +301,34 @@ public final class SearchParameters { } + private SearchParameters(final Long officeId, final String externalId, final String name, final String hierarchy, + final String firstname, final String lastname, final Integer offset, final Integer limit, final String orderBy, + final String sortOrder, final Long staffId, final String accountNo, final Long loanId, final Long savingsId, + final Boolean orphansOnly, boolean isSelfUser) { + this.sqlSearch = null; + this.officeId = officeId; + this.externalId = externalId; + this.name = name; + this.hierarchy = hierarchy; + this.firstname = firstname; + this.lastname = lastname; + this.offset = offset; + this.limit = limit; + this.orderBy = orderBy; + this.sortOrder = sortOrder; + this.staffId = staffId; + this.accountNo = accountNo; + this.loanId = loanId; + this.savingsId = savingsId; + this.orphansOnly = orphansOnly; + this.currencyCode = null; + this.provisioningEntryId = null; + this.productId = null; + this.categoryId = null; + this.isSelfUser = isSelfUser; + this.status = null; + } + private SearchParameters(final Long provisioningEntryId, final Long officeId, final Long productId, final Long categoryId, final Integer offset, final Integer limit) { this.sqlSearch = null; 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 index ae5f1343a..999f75258 100644 --- 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 @@ -85,7 +85,7 @@ public class SQLBuilder { "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")) { + && !operator.equals("<>") && !operator.equals("LIKE") && !operator.equals("like") && !operator.toLowerCase().equals("is")) { // 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) diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/api/CentersApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/api/CentersApiResource.java index 2456c0489..4db54815a 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/api/CentersApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/api/CentersApiResource.java @@ -209,8 +209,8 @@ public class CentersApiResource { } final PaginationParameters parameters = PaginationParameters.instance(paged, offset, limit, orderBy, sortOrder); final Boolean isOrphansOnly = false; - final SearchParameters searchParameters = SearchParameters.forGroups(sqlSearch, officeId, staffId, externalId, name, hierarchy, - offset, limit, orderBy, sortOrder, isOrphansOnly); + final SearchParameters searchParameters = SearchParameters.forGroups(officeId, staffId, externalId, name, hierarchy, offset, limit, + orderBy, sortOrder, isOrphansOnly); if (parameters.isPaged()) { final Page centers = this.centerReadPlatformService.retrievePagedAll(searchParameters, parameters); return this.toApiJsonSerializer.serialize(settings, centers, GroupingTypesApiConstants.CENTER_RESPONSE_DATA_PARAMETERS); diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/api/GroupsApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/api/GroupsApiResource.java index 3ac463f52..e86c07496 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/api/GroupsApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/api/GroupsApiResource.java @@ -229,7 +229,6 @@ public class GroupsApiResource { @ApiResponses({ @ApiResponse(responseCode = "200", description = "OK", content = @Content(schema = @Schema(implementation = GroupsApiResourceSwagger.GetGroupsResponse.class))) }) public String retrieveAll(@Context final UriInfo uriInfo, - @QueryParam("sqlSearch") @Parameter(description = "sqlSearch") final String sqlSearch, @QueryParam("officeId") @Parameter(description = "officeId") final Long officeId, @QueryParam("staffId") @Parameter(description = "staffId") final Long staffId, @QueryParam("externalId") @Parameter(description = "externalId") final String externalId, @@ -246,8 +245,8 @@ public class GroupsApiResource { final PaginationParameters parameters = PaginationParameters.instance(paged, offset, limit, orderBy, sortOrder); final ApiRequestJsonSerializationSettings settings = this.apiRequestParameterHelper.process(uriInfo.getQueryParameters()); - final SearchParameters searchParameters = SearchParameters.forGroups(sqlSearch, officeId, staffId, externalId, name, hierarchy, - offset, limit, orderBy, sortOrder, orphansOnly); + final SearchParameters searchParameters = SearchParameters.forGroups(officeId, staffId, externalId, name, hierarchy, offset, limit, + orderBy, sortOrder, orphansOnly); if (parameters.isPaged()) { final Page groups = this.groupReadPlatformService.retrievePagedAll(searchParameters, parameters); return this.toApiJsonSerializer.serialize(settings, groups, GroupingTypesApiConstants.GROUP_RESPONSE_DATA_PARAMETERS); diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/service/GroupReadPlatformServiceImpl.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/service/GroupReadPlatformServiceImpl.java index 1ab747b47..963247ea0 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/service/GroupReadPlatformServiceImpl.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/group/service/GroupReadPlatformServiceImpl.java @@ -26,10 +26,8 @@ import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Set; -import org.apache.commons.lang3.StringUtils; import org.apache.fineract.infrastructure.codes.data.CodeValueData; import org.apache.fineract.infrastructure.codes.service.CodeValueReadPlatformService; -import org.apache.fineract.infrastructure.core.api.ApiParameterHelper; import org.apache.fineract.infrastructure.core.data.PaginationParameters; import org.apache.fineract.infrastructure.core.data.PaginationParametersDataValidator; import org.apache.fineract.infrastructure.core.domain.JdbcSupport; @@ -39,7 +37,7 @@ import org.apache.fineract.infrastructure.core.service.RoutingDataSource; import org.apache.fineract.infrastructure.core.service.SearchParameters; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; import org.apache.fineract.infrastructure.security.utils.ColumnValidator; -import org.apache.fineract.infrastructure.security.utils.SQLInjectionValidator; +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; @@ -148,14 +146,10 @@ public class GroupReadPlatformServiceImpl implements GroupReadPlatformService { final StringBuilder sqlBuilder = new StringBuilder(200); sqlBuilder.append("select SQL_CALC_FOUND_ROWS "); sqlBuilder.append(this.allGroupTypesDataMapper.schema()); - sqlBuilder.append(" where o.hierarchy like ?"); - List paramList = new ArrayList<>(Arrays.asList(hierarchySearchString)); - final String extraCriteria = getGroupExtraCriteria(this.allGroupTypesDataMapper.schema(), paramList, searchParameters); - this.columnValidator.validateSqlInjection(sqlBuilder.toString(), extraCriteria); - if (StringUtils.isNotBlank(extraCriteria)) { - sqlBuilder.append(" and (").append(extraCriteria).append(")"); - } + final SQLBuilder extraCriteria = getGroupExtraCriteria(this.allGroupTypesDataMapper.schema(), searchParameters); + extraCriteria.addCriteria(" o.hierarchy like ", hierarchySearchString); + sqlBuilder.append(" ").append(extraCriteria.getSQLTemplate()); if (parameters.isOrderByRequested()) { sqlBuilder.append(" order by ").append(searchParameters.getOrderBy()).append(' ').append(searchParameters.getSortOrder()); this.columnValidator.validateSqlInjection(sqlBuilder.toString(), searchParameters.getOrderBy(), @@ -170,7 +164,7 @@ public class GroupReadPlatformServiceImpl implements GroupReadPlatformService { } final String sqlCountRows = "SELECT FOUND_ROWS()"; - return this.paginationHelper.fetchPage(this.jdbcTemplate, sqlCountRows, sqlBuilder.toString(), paramList.toArray(), + return this.paginationHelper.fetchPage(this.jdbcTemplate, sqlCountRows, sqlBuilder.toString(), extraCriteria.getArguments(), this.allGroupTypesDataMapper); } @@ -183,15 +177,11 @@ public class GroupReadPlatformServiceImpl implements GroupReadPlatformService { final StringBuilder sqlBuilder = new StringBuilder(200); sqlBuilder.append("select "); sqlBuilder.append(this.allGroupTypesDataMapper.schema()); - sqlBuilder.append(" where o.hierarchy like ?"); - List paramList = new ArrayList<>(Arrays.asList(hierarchySearchString)); - if (searchParameters != null) { - final String extraCriteria = getGroupExtraCriteria(this.allGroupTypesDataMapper.schema(), paramList, searchParameters); + final SQLBuilder extraCriteria = getGroupExtraCriteria(this.allGroupTypesDataMapper.schema(), searchParameters); + extraCriteria.addCriteria("o.hierarchy like ", hierarchySearchString); + + sqlBuilder.append(" ").append(extraCriteria.getSQLTemplate()); - if (StringUtils.isNotBlank(extraCriteria)) { - sqlBuilder.append(" and (").append(extraCriteria).append(")"); - } - } if (parameters != null) { if (parameters.isOrderByRequested()) { sqlBuilder.append(parameters.orderBySql()); @@ -203,68 +193,39 @@ public class GroupReadPlatformServiceImpl implements GroupReadPlatformService { this.columnValidator.validateSqlInjection(sqlBuilder.toString(), parameters.limitSql()); } } - return this.jdbcTemplate.query(sqlBuilder.toString(), this.allGroupTypesDataMapper, paramList.toArray()); + return this.jdbcTemplate.query(sqlBuilder.toString(), this.allGroupTypesDataMapper, extraCriteria.getArguments()); } // 'g.' preffix because of ERROR 1052 (23000): Column 'column_name' in where // clause is ambiguous // caused by the same name of columns in m_office and m_group tables - private String getGroupExtraCriteria(String schemaSql, List paramList, final SearchParameters searchCriteria) { + private SQLBuilder getGroupExtraCriteria(String schemaSql, final SearchParameters searchCriteria) { - StringBuilder extraCriteria = new StringBuilder(200); - extraCriteria.append(" and g.level_Id = ").append(GroupTypes.GROUP.getId()); - String sqlSearch = searchCriteria.getSqlSearch(); - if (sqlSearch != null) { - SQLInjectionValidator.validateSQLInput(sqlSearch); - sqlSearch = sqlSearch.replace(" display_name ", " g.display_name "); - sqlSearch = sqlSearch.replace("display_name ", "g.display_name "); - extraCriteria.append(" and ( ").append(sqlSearch).append(") "); - this.columnValidator.validateSqlInjection(schemaSql, sqlSearch); + SQLBuilder extraCriteria = new SQLBuilder(); + if (searchCriteria == null) { + return extraCriteria; } + extraCriteria.addCriteria("g.level_Id = ", GroupTypes.GROUP.getId()); - final Long officeId = searchCriteria.getOfficeId(); - if (officeId != null) { - paramList.add(officeId); - extraCriteria.append(" and g.office_id = ? "); - } + extraCriteria.addNonNullCriteria(" g.office_id = ", searchCriteria.getOfficeId()); - final String externalId = searchCriteria.getExternalId(); - if (externalId != null) { - paramList.add(ApiParameterHelper.sqlEncodeString(externalId)); - extraCriteria.append(" and g.external_id = ? "); - } + extraCriteria.addNonNullCriteria(" g.external_id =", searchCriteria.getExternalId()); final String name = searchCriteria.getName(); if (name != null) { - paramList.add("%" + name + "%"); - extraCriteria.append(" and g.display_name like ? "); + extraCriteria.addNonNullCriteria("g.display_name like", "%" + name + "%"); } final String hierarchy = searchCriteria.getHierarchy(); if (hierarchy != null) { - paramList.add(ApiParameterHelper.sqlEncodeString(hierarchy + "%")); - extraCriteria.append(" and o.hierarchy like ? "); - } - - if (searchCriteria.isStaffIdPassed()) { - paramList.add(searchCriteria.getStaffId()); - extraCriteria.append(" and g.staff_id = ? "); - } - - if (StringUtils.isNotBlank(extraCriteria.toString())) { - extraCriteria.delete(0, 4); - } - - final Long staffId = searchCriteria.getStaffId(); - if (staffId != null) { - paramList.add(staffId); - extraCriteria.append(" and g.staff_id = ? "); + extraCriteria.addNonNullCriteria("o.hierarchy like ? ", hierarchy + "%"); } + extraCriteria.addNonNullCriteria("g.staff_id =", searchCriteria.getStaffId()); if (searchCriteria.isOrphansOnly()) { - extraCriteria.append(" and g.parent_id IS NULL"); + extraCriteria.addNonNullCriteria("g.parent_id IS", "NULL"); } - return extraCriteria.toString(); + return extraCriteria; } @Override