=== modified file 'dhis-2/dhis-api/src/main/java/org/hisp/dhis/sqlview/SqlView.java' --- dhis-2/dhis-api/src/main/java/org/hisp/dhis/sqlview/SqlView.java 2015-02-12 09:12:38 +0000 +++ dhis-2/dhis-api/src/main/java/org/hisp/dhis/sqlview/SqlView.java 2015-02-12 19:37:50 +0000 @@ -56,12 +56,17 @@ extends BaseIdentifiableObject { public static final String PREFIX_VIEWNAME = "_view"; - - private static final String CRITERIA_SEP = ":"; + public static final String REGEX_SELECT_QUERY = "^(?i)\\s*select\\s{1,}.+$"; public static final Set PROTECTED_TABLES = Sets.newHashSet( "users", "userinfo", "trackedentityinstance", "trackedentityattribute", "trackedentityattributevalue", "relationship" ); + public static final Set ILLEGAL_KEYWORDS = Sets.newHashSet( "delete", "alter", "update", + "create", "drop", "commit", "createdb", "createuser", "insert", "rename", "replace", "restore", "write" ); + + private static final String CRITERIA_SEP = ":"; + private static final String REGEX_SEP = "|"; + // ------------------------------------------------------------------------- // Variables // ------------------------------------------------------------------------- @@ -78,7 +83,6 @@ public SqlView() { - } public SqlView( String name, String sqlQuery, boolean query ) @@ -134,6 +138,34 @@ return map; } + + public static String getProtectedTablesRegex() + { + StringBuffer regex = new StringBuffer( "^.*?(" ); + + for ( String table : PROTECTED_TABLES ) + { + regex.append( table ).append( REGEX_SEP ); + } + + regex.delete( regex.length() - 1, regex.length() ); + + return regex.append( ").*$" ).toString(); + } + + public static String getIllegalKeywordsRegex() + { + StringBuffer regex = new StringBuffer( "^.*?(" ); + + for ( String word : ILLEGAL_KEYWORDS ) + { + regex.append( word ).append( REGEX_SEP ); + } + + regex.delete( regex.length() - 1, regex.length() ); + + return regex.append( ").*$" ).toString(); + } public SqlView cleanSqlQuery() { === modified file 'dhis-2/dhis-api/src/main/java/org/hisp/dhis/sqlview/SqlViewService.java' --- dhis-2/dhis-api/src/main/java/org/hisp/dhis/sqlview/SqlViewService.java 2015-02-11 22:50:44 +0000 +++ dhis-2/dhis-api/src/main/java/org/hisp/dhis/sqlview/SqlViewService.java 2015-02-12 19:37:50 +0000 @@ -30,8 +30,11 @@ import java.util.Collection; import java.util.Map; +import java.util.Set; +import java.util.regex.Pattern; import org.hisp.dhis.common.Grid; +import org.hisp.dhis.common.IllegalQueryException; /** * @author Dang Duy Hieu @@ -39,8 +42,10 @@ */ public interface SqlViewService { - String ID = SqlViewService.class.getName(); - + final String ID = SqlViewService.class.getName(); + final String VARIABLE_EXPRESSION = "\\$\\{(\\w+)\\}"; + final Pattern VARIABLE_PATTERN = Pattern.compile( VARIABLE_EXPRESSION ); + // ------------------------------------------------------------------------- // SqlView // ------------------------------------------------------------------------- @@ -92,7 +97,7 @@ Grid getSqlViewGrid( SqlView sqlView, Map criteria, Map variables ); /** - * Substitutes the given SQL string with the given variables. SQL variables + * Substitutes the given SQL query string with the given variables. SQL variables * are on the format ${key}. * * @param sql the SQL string. @@ -101,5 +106,30 @@ */ String substituteSql( String sql, Map variables ); + /** + * Validates the given SQL view. Checks include: + * + *
    + *
  • All necessary variables are supplied.
    • + *
  • Variable keys and values do not contain null values.
    • + *
  • Invalid tables are not present in SQL query.
    • + *
+ * + * @param sqlView the SQL view. + * @param criteria the criteria. + * @param variables the variables. + * @throws IllegalQueryException if SQL view is invalid. + */ + void validateSqlView( SqlView sqlView, Map criteria, Map variables ) + throws IllegalQueryException; + + /** + * Returns the variables contained in the given SQL. + * + * @param sql the SQL query string. + * @return a set of variable keys. + */ + Set getVariables( String sql ); + String testSqlGrammar( String sql ); } === modified file 'dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/sqlview/DefaultSqlViewService.java' --- dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/sqlview/DefaultSqlViewService.java 2015-02-12 09:51:39 +0000 +++ dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/sqlview/DefaultSqlViewService.java 2015-02-12 19:37:50 +0000 @@ -29,10 +29,16 @@ */ import java.util.Collection; +import java.util.HashSet; import java.util.Map; +import java.util.Set; +import java.util.regex.Matcher; import org.apache.commons.lang.StringUtils; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.hisp.dhis.common.Grid; +import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.system.grid.ListGrid; import org.springframework.transaction.annotation.Transactional; @@ -43,6 +49,8 @@ public class DefaultSqlViewService implements SqlViewService { + private static final Log log = LogFactory.getLog( DefaultSqlViewService.class ); + // ------------------------------------------------------------------------- // Dependencies // ------------------------------------------------------------------------- @@ -154,6 +162,8 @@ Grid grid = new ListGrid(); grid.setTitle( sqlView.getName() ); + validateSqlView( sqlView, criteria, variables ); + if ( sqlView.isQuery() ) { final String sql = substituteSql( sqlView.getSqlQuery(), variables ); @@ -194,6 +204,78 @@ } @Override + public Set getVariables( String sql ) + { + Set variables = new HashSet<>(); + + Matcher matcher = VARIABLE_PATTERN.matcher( sql ); + + while ( matcher.find() ) + { + variables.add( matcher.group( 1 ) ); + } + + return variables; + } + + @Override + public void validateSqlView( SqlView sqlView, Map criteria, Map variables ) + throws IllegalQueryException + { + String violation = null; + + if ( sqlView == null || sqlView.getSqlQuery() == null ) + { + violation = "SQL query is null"; + } + + final Set sqlVars = getVariables( sqlView.getSqlQuery() ); + final String sql = sqlView.getSqlQuery(); + + if ( !sqlView.getSqlQuery().matches( SqlView.REGEX_SELECT_QUERY ) ) + { + violation = "SQL query must be a select query"; + } + + if ( sql.contains( ";" ) && !sql.trim().endsWith( ";" ) ) + { + violation = "SQL query can only contain a single semi-colon at the end of the query"; + } + + if ( variables != null && variables.keySet().contains( null ) ) + { + violation = "Variables contains null key"; + } + + if ( variables != null && variables.values().contains( null ) ) + { + violation = "Variables contains null value"; + } + + if ( sqlView.isQuery() && !sqlVars.isEmpty() && ( variables == null || !variables.keySet().containsAll( sqlVars ) ) ) + { + violation = "SQL query contains variables which were not supplied in request: " + sqlVars; + } + + if ( sql.matches( SqlView.getProtectedTablesRegex() ) ) + { + violation = "SQL query contains references to protected tables"; + } + + if ( sql.matches( SqlView.getIllegalKeywordsRegex() ) ) + { + violation = "SQL query contains illegal keywords"; + } + + if ( violation != null ) + { + log.warn( "Validation failed: " + violation ); + + throw new IllegalQueryException( violation ); + } + } + + @Override public String testSqlGrammar( String sql ) { return sqlViewStore.testSqlGrammar( sql ); === modified file 'dhis-2/dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/sqlview/SqlViewServiceTest.java' --- dhis-2/dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/sqlview/SqlViewServiceTest.java 2015-02-11 22:50:44 +0000 +++ dhis-2/dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/sqlview/SqlViewServiceTest.java 2015-02-12 19:37:50 +0000 @@ -35,11 +35,15 @@ import java.util.HashMap; import java.util.Map; +import java.util.Set; import org.hisp.dhis.DhisSpringTest; +import org.hisp.dhis.common.IllegalQueryException; import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; +import com.google.common.collect.Sets; + /** * @author Dang Duy Hieu */ @@ -150,7 +154,7 @@ } @Test - public void testMakeUpForQueryStatement() + public void testCleanSqlQuery() { SqlView sqlViewA = createSqlView( 'A', SQL1 ); @@ -205,4 +209,70 @@ assertEquals( expected, actual ); } + + @Test + public void testGetVariables() + { + String sql = "select * from dataelement where valuetype = '${valueType} and aggregationtype = '${aggregationType}'"; + + Set expected = Sets.newHashSet( "valueType", "aggregationType" ); + + Set actual = sqlViewService.getVariables( sql ); + + assertEquals( expected, actual ); + } + + @Test( expected = IllegalQueryException.class ) + public void testValidateIllegalKeywords() + { + SqlView sqlView = new SqlView( "Name", "delete * from dataelement", true ); + + sqlViewService.validateSqlView( sqlView, null, null ); + } + + @Test( expected = IllegalQueryException.class ) + public void testValidateProtectedTables() + { + SqlView sqlView = new SqlView( "Name", "select * from userinfo", true ); + + sqlViewService.validateSqlView( sqlView, null, null ); + } + + @Test( expected = IllegalQueryException.class ) + public void testValidateMissingVariables() + { + SqlView sqlView = new SqlView( "Name", "select * from dataelement where valueType = '${valueType}' and aggregationtype = '${aggregationType}'", true ); + + Map variables = new HashMap<>(); + variables.put( "valueType", "int" ); + + sqlViewService.validateSqlView( sqlView, null, variables ); + } + + @Test( expected = IllegalQueryException.class ) + public void testValidateIllegalSemiColon() + { + SqlView sqlView = new SqlView( "Name", "select * from dataelement; delete from dataelement", true ); + + sqlViewService.validateSqlView( sqlView, null, null ); + } + + @Test( expected = IllegalQueryException.class ) + public void testValidateNotSelectQuery() + { + SqlView sqlView = new SqlView( "Name", "* from dataelement", true ); + + sqlViewService.validateSqlView( sqlView, null, null ); + } + + @Test + public void testValidateSuccess() + { + SqlView sqlView = new SqlView( "Name", "select * from dataelement where valueType = '${valueType}'", true ); + + Map variables = new HashMap<>(); + variables.put( "valueType", "int" ); + + sqlViewService.validateSqlView( sqlView, null, variables ); + } } === modified file 'dhis-2/dhis-web/dhis-web-maintenance/dhis-web-maintenance-dataadmin/src/main/java/org/hisp/dhis/dataadmin/action/sqlview/ValidateAddUpdateSqlViewAction.java' --- dhis-2/dhis-web/dhis-web-maintenance/dhis-web-maintenance-dataadmin/src/main/java/org/hisp/dhis/dataadmin/action/sqlview/ValidateAddUpdateSqlViewAction.java 2015-02-12 09:51:39 +0000 +++ dhis-2/dhis-web/dhis-web-maintenance/dhis-web-maintenance-dataadmin/src/main/java/org/hisp/dhis/dataadmin/action/sqlview/ValidateAddUpdateSqlViewAction.java 2015-02-12 19:37:50 +0000 @@ -28,13 +28,13 @@ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ +import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.i18n.I18n; +import org.hisp.dhis.sqlview.SqlView; import org.hisp.dhis.sqlview.SqlViewService; import com.opensymphony.xwork2.Action; -import static org.hisp.dhis.sqlview.SqlView.PROTECTED_TABLES; - /** * @author Dang Duy Hieu */ @@ -42,14 +42,7 @@ implements Action { private static final String ADD = "add"; - private static final String SEMICOLON = ";"; - private static final String SEP = "|"; - private static final String SPACE = " "; - private static final String INTO = " into "; - private static final String REGEX_SELECT_QUERY = "^(?i)\\s*select\\s{1,}.+$"; - private static final String PREFIX_REGEX_IGNORE_TABLES_QUERY = "^(?i).+((?<=[^\\d\\w])("; - private static final String SUFFIX_REGEX_IGNORE_TABLES_QUERY = ")(?=[^\\d\\w])).*$"; - + // ------------------------------------------------------------------------- // Dependencies // ------------------------------------------------------------------------- @@ -119,8 +112,6 @@ // Action implementation // ------------------------------------------------------------------------- - //TODO move to service layer and validate queries made in web api - @Override public String execute() { @@ -147,25 +138,17 @@ return INPUT; } - final String protectedTablesRegex = getProtectedTablesRegex(); - - for ( String s : sqlquery.split( SEMICOLON ) ) - { - String tmp = new String( s.toLowerCase() ).trim(); - - if ( !s.matches( REGEX_SELECT_QUERY ) || tmp.contains( INTO ) ) - { - message = i18n.getString( "sqlquery_is_invalid" ) + "
" + i18n.getString( "sqlquery_invalid_note" ); - - return INPUT; - } - - if ( tmp.concat( SPACE ).matches( protectedTablesRegex ) ) - { - message = i18n.getString( "sqlquery_is_not_allowed" ); - - return INPUT; - } + try + { + SqlView sqlView = new SqlView( name, sqlquery, false ); // Avoid variable check + + sqlViewService.validateSqlView( sqlView, null, null ); + } + catch ( IllegalQueryException ex ) + { + message = ex.getMessage(); + + return INPUT; } if ( !query ) @@ -180,30 +163,4 @@ return SUCCESS; } - - // ------------------------------------------------------------------------- - // Supportive methods - // ------------------------------------------------------------------------- - - private String getProtectedTablesRegex() - { - int i = 0; - int len = PROTECTED_TABLES.size(); - - StringBuffer ignoredRegex = new StringBuffer( PREFIX_REGEX_IGNORE_TABLES_QUERY ); - - for ( String table : PROTECTED_TABLES ) - { - ignoredRegex.append( table ); - - if ( ++i < len ) - { - ignoredRegex.append( SEP ); - } - } - - ignoredRegex.append( SUFFIX_REGEX_IGNORE_TABLES_QUERY ); - - return ignoredRegex.toString(); - } }