Improve permission checks in DemographicsService

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,7 +41,11 @@ 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);
     }
 
@@ -60,7 +65,10 @@ public static DemographicsSource getFromParts(Container c, User u, String label,
             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);
         }
@@ -103,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)
         {
@@ -131,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);
@@ -160,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);
@@ -177,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

@@ -20,6 +20,7 @@
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.jetbrains.annotations.NotNull;
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
@@ -159,7 +160,7 @@ public void validateCommand(Object form, Errors errors)
         }
 
         @Override
-        public URLHelper getSuccessURL(Object form)
+        public @NotNull URLHelper getSuccessURL(Object form)
         {
             return getContainer().getStartURL(getUser());
         }
@@ -281,7 +282,7 @@ public boolean handlePost(EnsureAssayFieldsForm form, BindException errors) thro
         }
 
         @Override
-        public URLHelper getSuccessURL(EnsureAssayFieldsForm form)
+        public @NotNull URLHelper getSuccessURL(EnsureAssayFieldsForm form)
         {
             if (form.getReturnActionURL() != null)
                 return form.getReturnActionURL();
@@ -384,7 +385,7 @@ private ContainerIncrementingTable getTable(String schema, String query, BindExc
         }
 
         @Override
-        public URLHelper getSuccessURL(SetTableIncrementForm form)
+        public @NotNull URLHelper getSuccessURL(SetTableIncrementForm form)
         {
             return getContainer().getStartURL(getUser());
         }
@@ -483,7 +484,7 @@ public boolean handlePost(Object form, BindException errors) throws Exception
         }
 
         @Override
-        public URLHelper getSuccessURL(Object form)
+        public @NotNull URLHelper getSuccessURL(Object form)
         {
             return getContainer().getStartURL(getUser());
         }
@@ -555,7 +556,7 @@ private void processContainer(Container c, String schema, String query)
         }
 
         @Override
-        public URLHelper getSuccessURL(SetTableIncrementForm form)
+        public @NotNull URLHelper getSuccessURL(SetTableIncrementForm form)
         {
             return getContainer().getStartURL(getUser());
         }
@@ -594,7 +595,7 @@ public boolean handlePost(Object form, BindException errors) throws Exception
         }
 
         @Override
-        public URLHelper getSuccessURL(Object form)
+        public @NotNull URLHelper getSuccessURL(Object form)
         {
             return getContainer().getStartURL(getUser());
         }
@@ -1308,7 +1309,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/LaboratoryManager.java

@@ -71,6 +71,7 @@
 import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -388,7 +389,7 @@ private void populateDefaultDataForTable(User u, Container c, String schema, Str
         List<Object> existingValues = new TableSelector(targetTable, PageFlowUtil.set(keyCol)).getArrayList(Object.class);
 
         //Now find the values from /Shared
-        Set<String> selectCols = new HashSet<>(columns);
+        Set<String> selectCols = new LinkedHashSet<>(columns);
         columns.add(keyCol);
 
         final List<Map<String, Object>> rows = new ArrayList<>();

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)
             {
@@ -458,11 +460,15 @@ 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)