← Back to team overview

dhis2-devs team mailing list archive

[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();
-    }
 }