fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From vorbur...@apache.org
Subject [fineract] branch develop updated: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Date Wed, 26 Aug 2020 00:01:35 GMT
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


The following commit(s) were added to refs/heads/develop by this push:
     new 7950ca3  FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
7950ca3 is described below

commit 7950ca3e58d10f1255701824fcc9ab505486a24c
Author: Manthan Surkar <manthan.surkar@gmail.com>
AuthorDate: Wed Aug 26 04:24:23 2020 +0530

    FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
---
 .../core/service/SearchParameters.java             | 38 ++++++++--
 .../infrastructure/security/utils/SQLBuilder.java  |  2 +-
 .../portfolio/group/api/CentersApiResource.java    |  4 +-
 .../portfolio/group/api/GroupsApiResource.java     |  5 +-
 .../service/GroupReadPlatformServiceImpl.java      | 85 ++++++----------------
 5 files changed, 61 insertions(+), 73 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 11e8af5..9a4d995 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 ae5f134..999f752 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 2456c04..4db5481 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<CenterData> 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 3ac463f..e86c074 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<GroupGeneralData> 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 1ab747b..963247e 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<Object> 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<Object> 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<Object> paramList,
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);
-        }
+    private SQLBuilder getGroupExtraCriteria(String schemaSql, final SearchParameters searchCriteria)
{
 
-        final Long officeId = searchCriteria.getOfficeId();
-        if (officeId != null) {
-            paramList.add(officeId);
-            extraCriteria.append(" and g.office_id = ? ");
+        SQLBuilder extraCriteria = new SQLBuilder();
+        if (searchCriteria == null) {
+            return extraCriteria;
         }
+        extraCriteria.addCriteria("g.level_Id = ", GroupTypes.GROUP.getId());
 
-        final String externalId = searchCriteria.getExternalId();
-        if (externalId != null) {
-            paramList.add(ApiParameterHelper.sqlEncodeString(externalId));
-            extraCriteria.append(" and g.external_id = ? ");
-        }
+        extraCriteria.addNonNullCriteria(" g.office_id = ", searchCriteria.getOfficeId());
+
+        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


Mime
View raw message