dhis2-devs team mailing list archive
-
dhis2-devs team
-
Mailing list archive
-
Message #35660
[Branch ~dhis2-devs-core/dhis2/trunk] Rev 18245: SQL view. Improved and centralized validation of views.
------------------------------------------------------------
revno: 18245
committer: Lars Helge Overland <larshelge@xxxxxxxxx>
branch nick: dhis2
timestamp: Thu 2015-02-12 20:37:50 +0100
message:
SQL view. Improved and centralized validation of views.
modified:
dhis-2/dhis-api/src/main/java/org/hisp/dhis/sqlview/SqlView.java
dhis-2/dhis-api/src/main/java/org/hisp/dhis/sqlview/SqlViewService.java
dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/sqlview/DefaultSqlViewService.java
dhis-2/dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/sqlview/SqlViewServiceTest.java
dhis-2/dhis-web/dhis-web-maintenance/dhis-web-maintenance-dataadmin/src/main/java/org/hisp/dhis/dataadmin/action/sqlview/ValidateAddUpdateSqlViewAction.java
--
lp:dhis2
https://code.launchpad.net/~dhis2-devs-core/dhis2/trunk
Your team DHIS 2 developers is subscribed to branch lp:dhis2.
To unsubscribe from this branch go to https://code.launchpad.net/~dhis2-devs-core/dhis2/trunk/+edit-subscription
=== 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<String> PROTECTED_TABLES = Sets.newHashSet( "users", "userinfo",
"trackedentityinstance", "trackedentityattribute", "trackedentityattributevalue", "relationship" );
+ public static final Set<String> 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<String, String> criteria, Map<String, String> 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<String, String> variables );
+ /**
+ * Validates the given SQL view. Checks include:
+ *
+ * <ul>
+ * <li>All necessary variables are supplied.</li>
+ * <li>Variable keys and values do not contain null values.</li>
+ * <li>Invalid tables are not present in SQL query.</li>
+ * </ul>
+ *
+ * @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<String, String> criteria, Map<String, String> variables )
+ throws IllegalQueryException;
+
+ /**
+ * Returns the variables contained in the given SQL.
+ *
+ * @param sql the SQL query string.
+ * @return a set of variable keys.
+ */
+ Set<String> 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<String> getVariables( String sql )
+ {
+ Set<String> 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<String, String> criteria, Map<String, String> variables )
+ throws IllegalQueryException
+ {
+ String violation = null;
+
+ if ( sqlView == null || sqlView.getSqlQuery() == null )
+ {
+ violation = "SQL query is null";
+ }
+
+ final Set<String> 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<String> expected = Sets.newHashSet( "valueType", "aggregationType" );
+
+ Set<String> 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<String, String> 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<String, String> 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" ) + "<br/>" + 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();
- }
}
