From 68d437dc55939a3c025bb6031f3090d1d0d2182e Mon Sep 17 00:00:00 2001 From: bbimber Date: Thu, 26 May 2022 06:54:26 -0700 Subject: [PATCH] Improve permission checking in DemographicsSources (#26) --- .../labkey/laboratory/DemographicsSource.java | 64 +++++++++++++++---- .../laboratory/LaboratoryController.java | 6 +- .../laboratory/LaboratoryServiceImpl.java | 12 +++- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/laboratory/src/org/labkey/laboratory/DemographicsSource.java b/laboratory/src/org/labkey/laboratory/DemographicsSource.java index 3d5e0847..32124688 100644 --- a/laboratory/src/org/labkey/laboratory/DemographicsSource.java +++ b/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 errors = new ArrayList(); + List 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; } diff --git a/laboratory/src/org/labkey/laboratory/LaboratoryController.java b/laboratory/src/org/labkey/laboratory/LaboratoryController.java index 8a740d28..be59db70 100644 --- a/laboratory/src/org/labkey/laboratory/LaboratoryController.java +++ b/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 diff --git a/laboratory/src/org/labkey/laboratory/LaboratoryServiceImpl.java b/laboratory/src/org/labkey/laboratory/LaboratoryServiceImpl.java index 3d7eb74f..dceff2eb 100644 --- a/laboratory/src/org/labkey/laboratory/LaboratoryServiceImpl.java +++ b/laboratory/src/org/labkey/laboratory/LaboratoryServiceImpl.java @@ -383,7 +383,9 @@ public Set 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> getAllDemographicsSources(User u) { Container c = ContainerManager.getForId(entry.getObjectId()); if (c == null || !c.hasPermission(u, ReadPermission.class)) + { continue; + } Set 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);