Improve permission checking in DemographicsSources (#26)

Author: bbimber

laboratory/src/org/labkey/laboratory/DemographicsSource.java

@@ -16,6 +16,7 @@
 import org.labkey.api.query.QueryService;
 import org.labkey.api.query.UserSchema;
 import org.labkey.api.security.User;
+import org.labkey.api.security.permissions.ReadPermission;
 import org.labkey.api.study.Dataset;
 import org.labkey.api.study.DatasetTable;
 
@@ -40,15 +41,21 @@ public DemographicsSource(String label, String containerId, String schemaName, S
 
     public static DemographicsSource getFromParts(Container c, User u, String label, String containerId, String schemaName, String queryName, String targetColumn) throws IllegalArgumentException
     {
-        DemographicsSource.validateKey(c, u, containerId, schemaName, queryName, targetColumn, label);
+        if (!isValidSource(c, u, containerId, schemaName, queryName, targetColumn, label))
+        {
+            return null;    
+        }
+        
         return new DemographicsSource(label, containerId, schemaName, queryName, targetColumn);
     }
 
     public static DemographicsSource getFromPropertyManager(Container c, User u, String key, String value) throws IllegalArgumentException
     {
         if (value == null)
+        {
             return null;
-
+        }
+        
         try
         {
             JSONObject json = new JSONObject(value);
@@ -58,7 +65,10 @@ public static DemographicsSource getFromPropertyManager(Container c, User u, Str
             String label = json.getString("label");
             String targetColumn = json.getString("targetColumn");
 
-            validateKey(c, u, containerId, schemaName, queryName, targetColumn, label);
+            if (!isValidSource(c, u, containerId, schemaName, queryName, targetColumn, label))
+            {
+                return null;
+            }
 
             return new DemographicsSource(label, containerId, schemaName, queryName, targetColumn);
         }
@@ -101,23 +111,39 @@ public JSONObject toJSON(Container c, User u, boolean includeTotals)
         return json;
     }
 
-    public static boolean validateKey(Container defaultContainer, User u, @Nullable String containerId, String schemaName, String queryName, String targetColumn, String label) throws IllegalArgumentException
+    private static boolean isValidSource(Container defaultContainer, User u, @Nullable String containerId, String schemaName, String queryName, String targetColumn, String label) throws IllegalArgumentException
     {
         Container target;
         if (containerId == null)
+        {
             target = defaultContainer;
+        }
         else
+        {
             target = ContainerManager.getForId(containerId);
+        }
 
         if (target == null)
+        {
             target = defaultContainer;
+        }
+        
+        if (!target.hasPermission(u, ReadPermission.class))
+        {
+            return false;
+        }
 
         UserSchema us = QueryService.get().getUserSchema(u, target, schemaName);
-        if (target == null)
+        if (us == null)
         {
             throw new IllegalArgumentException("Unknown schema in saved data source: " + schemaName);
         }
 
+        if (!us.canReadSchema())
+        {
+            return false;
+        }
+
         QueryDefinition qd = us.getQueryDefForTable(queryName);
         if (qd == null)
         {
@@ -129,19 +155,28 @@ public static boolean validateKey(Container defaultContainer, User u, @Nullable
             throw new IllegalArgumentException("Missing targetColumn");
         }
 
-        List<QueryException> errors = new ArrayList<QueryException>();
+        List<QueryException> errors = new ArrayList<>();
         TableInfo ti = qd.getTable(errors,  true);
-        if (errors.size() != 0 || ti == null)
+
+        if (!errors.isEmpty())
         {
             _log.error("Unable to create TableInfo for query: " + queryName + ". there were " + errors.size() + " errors");
             for (QueryException e : errors)
             {
                 _log.error(e.getMessage());
             }
-            if (errors.size() > 0)
-                throw new IllegalArgumentException("Unable to create table for query: " + queryName, errors.get(0));
-            else
-                throw new IllegalArgumentException("Unable to create table for query: " + queryName);
+
+            throw new IllegalArgumentException("Unable to create table for query: " + queryName, errors.get(0));
+        }
+
+        if (ti == null)
+        {
+            throw new IllegalArgumentException("Unable to create table for query: " + queryName);
+        }
+
+        if (!ti.hasPermission(u, ReadPermission.class))
+        {
+            return false;
         }
 
         ColumnInfo col = ti.getColumn(targetColumn);
@@ -158,6 +193,11 @@ public static boolean validateKey(Container defaultContainer, User u, @Nullable
             if (ti instanceof DatasetTable)
             {
                 Dataset ds = ((DatasetTable)ti).getDataset();
+                if (!ds.hasPermission(u, ReadPermission.class))
+                {
+                    return false;
+                }
+
                 if (!(ds.isDemographicData() && ds.getStudy().getSubjectColumnName().equalsIgnoreCase(col.getName())))
                 {
                     throw new IllegalArgumentException("Target column is not a key field: " + targetColumn);
@@ -175,7 +215,9 @@ public static boolean validateKey(Container defaultContainer, User u, @Nullable
         }
 
         if (StringUtils.trimToNull(label) == null)
+        {
             throw new IllegalArgumentException("Label must not be blank");
+        }
 
         return true;
     }

laboratory/src/org/labkey/laboratory/LaboratoryController.java

@@ -1308,7 +1308,11 @@ public ApiResponse execute(SetDataSourcesForm form, BindException errors)
                     return null;
                 }
 
-                sources.add(DemographicsSource.getFromParts(getContainer(), getUser(), label, containerId, schemaName, queryName, targetColumn));
+                DemographicsSource s = DemographicsSource.getFromParts(getContainer(), getUser(), label, containerId, schemaName, queryName, targetColumn);
+                if (s != null)
+                {
+                    sources.add(s);
+                }
             }
 
             try

laboratory/src/org/labkey/laboratory/LaboratoryServiceImpl.java

@@ -383,7 +383,9 @@ public Set<DemographicsSource> getDemographicsSources(Container c, User u) throw
             {
                 DemographicsSource source = DemographicsSource.getFromPropertyManager(target, u, key, properties.get(key));
                 if (source != null)
+                {
                     qds.add(source);
+                }
             }
             catch (IllegalArgumentException e)
             {
@@ -450,18 +452,26 @@ public Map<Container, Set<DemographicsSource>> getAllDemographicsSources(User u)
         {
             Container c = ContainerManager.getForId(entry.getObjectId());
             if (c == null || !c.hasPermission(u, ReadPermission.class))
+            {
                 continue;
+            }
 
             Set<DemographicsSource> set = map.get(c);
             if (set == null)
+            {
                 set = new HashSet<>();
+            }
 
             DemographicsSource source = DemographicsSource.getFromPropertyManager(c, u, entry.getKey(), entry.getValue());
             if (source != null)
+            {
                 set.add(source);
+            }
 
-            if (set.size() > 0)
+            if (!set.isEmpty())
+            {
                 map.put(c, set);
+            }
         }
 
         return Collections.unmodifiableMap(map);