Skip to content

Commit d9dd395

Browse files
bbimberbbimber
authored
Improve permission checks in DemographicsService (#128)
1 parent 8f05fde commit d9dd395

File tree

4 files changed

+70
-18
lines changed

4 files changed

+70
-18
lines changed

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

Lines changed: 50 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.labkey.api.query.QueryService;
1717
import org.labkey.api.query.UserSchema;
1818
import org.labkey.api.security.User;
19+
import org.labkey.api.security.permissions.ReadPermission;
1920
import org.labkey.api.study.Dataset;
2021
import org.labkey.api.study.DatasetTable;
2122

@@ -40,7 +41,11 @@ public DemographicsSource(String label, String containerId, String schemaName, S
4041

4142
public static DemographicsSource getFromParts(Container c, User u, String label, String containerId, String schemaName, String queryName, String targetColumn) throws IllegalArgumentException
4243
{
43-
DemographicsSource.validateKey(c, u, containerId, schemaName, queryName, targetColumn, label);
44+
if (!isValidSource(c, u, containerId, schemaName, queryName, targetColumn, label))
45+
{
46+
return null;
47+
}
48+
4449
return new DemographicsSource(label, containerId, schemaName, queryName, targetColumn);
4550
}
4651

@@ -60,7 +65,10 @@ public static DemographicsSource getFromParts(Container c, User u, String label,
6065
String label = json.getString("label");
6166
String targetColumn = json.getString("targetColumn");
6267

63-
validateKey(c, u, containerId, schemaName, queryName, targetColumn, label);
68+
if (!isValidSource(c, u, containerId, schemaName, queryName, targetColumn, label))
69+
{
70+
return null;
71+
}
6472

6573
return new DemographicsSource(label, containerId, schemaName, queryName, targetColumn);
6674
}
@@ -103,23 +111,39 @@ public JSONObject toJSON(Container c, User u, boolean includeTotals)
103111
return json;
104112
}
105113

106-
public static boolean validateKey(Container defaultContainer, User u, @Nullable String containerId, String schemaName, String queryName, String targetColumn, String label) throws IllegalArgumentException
114+
private static boolean isValidSource(Container defaultContainer, User u, @Nullable String containerId, String schemaName, String queryName, String targetColumn, String label) throws IllegalArgumentException
107115
{
108116
Container target;
109117
if (containerId == null)
118+
{
110119
target = defaultContainer;
120+
}
111121
else
122+
{
112123
target = ContainerManager.getForId(containerId);
124+
}
113125

114126
if (target == null)
127+
{
115128
target = defaultContainer;
129+
}
130+
131+
if (!target.hasPermission(u, ReadPermission.class))
132+
{
133+
return false;
134+
}
116135

117136
UserSchema us = QueryService.get().getUserSchema(u, target, schemaName);
118-
if (target == null)
137+
if (us == null)
119138
{
120139
throw new IllegalArgumentException("Unknown schema in saved data source: " + schemaName);
121140
}
122141

142+
if (!us.canReadSchema())
143+
{
144+
return false;
145+
}
146+
123147
QueryDefinition qd = us.getQueryDefForTable(queryName);
124148
if (qd == null)
125149
{
@@ -131,19 +155,28 @@ public static boolean validateKey(Container defaultContainer, User u, @Nullable
131155
throw new IllegalArgumentException("Missing targetColumn");
132156
}
133157

134-
List<QueryException> errors = new ArrayList<QueryException>();
158+
List<QueryException> errors = new ArrayList<>();
135159
TableInfo ti = qd.getTable(errors, true);
136-
if (errors.size() != 0 || ti == null)
160+
161+
if (!errors.isEmpty())
137162
{
138163
_log.error("Unable to create TableInfo for query: " + queryName + ". there were " + errors.size() + " errors");
139164
for (QueryException e : errors)
140165
{
141166
_log.error(e.getMessage());
142167
}
143-
if (errors.size() > 0)
144-
throw new IllegalArgumentException("Unable to create table for query: " + queryName, errors.get(0));
145-
else
146-
throw new IllegalArgumentException("Unable to create table for query: " + queryName);
168+
169+
throw new IllegalArgumentException("Unable to create table for query: " + queryName, errors.get(0));
170+
}
171+
172+
if (ti == null)
173+
{
174+
throw new IllegalArgumentException("Unable to create table for query: " + queryName);
175+
}
176+
177+
if (!ti.hasPermission(u, ReadPermission.class))
178+
{
179+
return false;
147180
}
148181

149182
ColumnInfo col = ti.getColumn(targetColumn);
@@ -160,6 +193,11 @@ public static boolean validateKey(Container defaultContainer, User u, @Nullable
160193
if (ti instanceof DatasetTable)
161194
{
162195
Dataset ds = ((DatasetTable)ti).getDataset();
196+
if (!ds.hasPermission(u, ReadPermission.class))
197+
{
198+
return false;
199+
}
200+
163201
if (!(ds.isDemographicData() && ds.getStudy().getSubjectColumnName().equalsIgnoreCase(col.getName())))
164202
{
165203
throw new IllegalArgumentException("Target column is not a key field: " + targetColumn);
@@ -177,7 +215,9 @@ public static boolean validateKey(Container defaultContainer, User u, @Nullable
177215
}
178216

179217
if (StringUtils.trimToNull(label) == null)
218+
{
180219
throw new IllegalArgumentException("Label must not be blank");
220+
}
181221

182222
return true;
183223
}

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.apache.commons.lang3.StringUtils;
2121
import org.apache.logging.log4j.LogManager;
2222
import org.apache.logging.log4j.Logger;
23+
import org.jetbrains.annotations.NotNull;
2324
import org.json.JSONArray;
2425
import org.json.JSONException;
2526
import org.json.JSONObject;
@@ -159,7 +160,7 @@ public void validateCommand(Object form, Errors errors)
159160
}
160161

161162
@Override
162-
public URLHelper getSuccessURL(Object form)
163+
public @NotNull URLHelper getSuccessURL(Object form)
163164
{
164165
return getContainer().getStartURL(getUser());
165166
}
@@ -281,7 +282,7 @@ public boolean handlePost(EnsureAssayFieldsForm form, BindException errors) thro
281282
}
282283

283284
@Override
284-
public URLHelper getSuccessURL(EnsureAssayFieldsForm form)
285+
public @NotNull URLHelper getSuccessURL(EnsureAssayFieldsForm form)
285286
{
286287
if (form.getReturnActionURL() != null)
287288
return form.getReturnActionURL();
@@ -384,7 +385,7 @@ private ContainerIncrementingTable getTable(String schema, String query, BindExc
384385
}
385386

386387
@Override
387-
public URLHelper getSuccessURL(SetTableIncrementForm form)
388+
public @NotNull URLHelper getSuccessURL(SetTableIncrementForm form)
388389
{
389390
return getContainer().getStartURL(getUser());
390391
}
@@ -483,7 +484,7 @@ public boolean handlePost(Object form, BindException errors) throws Exception
483484
}
484485

485486
@Override
486-
public URLHelper getSuccessURL(Object form)
487+
public @NotNull URLHelper getSuccessURL(Object form)
487488
{
488489
return getContainer().getStartURL(getUser());
489490
}
@@ -555,7 +556,7 @@ private void processContainer(Container c, String schema, String query)
555556
}
556557

557558
@Override
558-
public URLHelper getSuccessURL(SetTableIncrementForm form)
559+
public @NotNull URLHelper getSuccessURL(SetTableIncrementForm form)
559560
{
560561
return getContainer().getStartURL(getUser());
561562
}
@@ -594,7 +595,7 @@ public boolean handlePost(Object form, BindException errors) throws Exception
594595
}
595596

596597
@Override
597-
public URLHelper getSuccessURL(Object form)
598+
public @NotNull URLHelper getSuccessURL(Object form)
598599
{
599600
return getContainer().getStartURL(getUser());
600601
}
@@ -1308,7 +1309,11 @@ public ApiResponse execute(SetDataSourcesForm form, BindException errors)
13081309
return null;
13091310
}
13101311

1311-
sources.add(DemographicsSource.getFromParts(getContainer(), getUser(), label, containerId, schemaName, queryName, targetColumn));
1312+
DemographicsSource s = DemographicsSource.getFromParts(getContainer(), getUser(), label, containerId, schemaName, queryName, targetColumn);
1313+
if (s != null)
1314+
{
1315+
sources.add(s);
1316+
}
13121317
}
13131318

13141319
try

‎laboratory/src/org/labkey/laboratory/LaboratoryManager.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@
7171
import java.util.Date;
7272
import java.util.HashMap;
7373
import java.util.HashSet;
74+
import java.util.LinkedHashSet;
7475
import java.util.List;
7576
import java.util.Map;
7677
import java.util.Set;
@@ -388,7 +389,7 @@ private void populateDefaultDataForTable(User u, Container c, String schema, Str
388389
List<Object> existingValues = new TableSelector(targetTable, PageFlowUtil.set(keyCol)).getArrayList(Object.class);
389390

390391
//Now find the values from /Shared
391-
Set<String> selectCols = new HashSet<>(columns);
392+
Set<String> selectCols = new LinkedHashSet<>(columns);
392393
columns.add(keyCol);
393394

394395
final List<Map<String, Object>> rows = new ArrayList<>();

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,9 @@ public Set<DemographicsSource> getDemographicsSources(Container c, User u) throw
383383
{
384384
DemographicsSource source = DemographicsSource.getFromPropertyManager(target, u, key, properties.get(key));
385385
if (source != null)
386+
{
386387
qds.add(source);
388+
}
387389
}
388390
catch (IllegalArgumentException e)
389391
{
@@ -458,11 +460,15 @@ public Map<Container, Set<DemographicsSource>> getAllDemographicsSources(User u)
458460
{
459461
Container c = ContainerManager.getForId(entry.getObjectId());
460462
if (c == null || !c.hasPermission(u, ReadPermission.class))
463+
{
461464
continue;
465+
}
462466

463467
Set<DemographicsSource> set = map.get(c);
464468
if (set == null)
469+
{
465470
set = new HashSet<>();
471+
}
466472

467473
DemographicsSource source = DemographicsSource.getFromPropertyManager(c, u, entry.getKey(), entry.getValue());
468474
if (source == null)

0 commit comments

Comments
 (0)