Skip to content

Commit e873689

Browse files
labkey-adamlabkey-adam
authored
Log error if we attempt to cache a "bad" value (#6794)
1 parent f23aa37 commit e873689

4 files changed

Lines changed: 73 additions & 49 deletions

File tree

‎api/src/org/labkey/api/cache/CacheManager.java‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public class CacheManager
6060
public static final int DEFAULT_CACHE_SIZE = 5000;
6161

6262
// Set useCache = false to completely disable all caching... and slow your server to a near halt. Possibly useful for
63-
// reproducing CacheLoader re-entrancy problems, but not much else.
63+
// reproducing CacheLoader reentrancy problems, but not much else.
6464
private static final boolean useCache = true;
6565
private static final CacheProvider PROVIDER = useCache ? EhCacheProvider.getInstance() : new NoopCacheProvider();
6666

@@ -176,7 +176,7 @@ public static void shutdown()
176176

177177
private static final Set<Class<?>> CLASSES = new HashSet<>();
178178

179-
// Validate a cached value. For now, just log warnings for mutable collections.
179+
// Validate a cached value. Log errors for mutable collections/arrays and for values holding Container or User objects.
180180
public static <V> void validate(String debugName, @Nullable V value)
181181
{
182182
if (value instanceof Wrapper<?>)
@@ -186,14 +186,11 @@ public static <V> void validate(String debugName, @Nullable V value)
186186

187187
if (null != description)
188188
{
189-
LOG.warn("{} attempted to cache {}, which could be mutated by callers!", debugName, description);
189+
LOG.error("{} attempted to cache {}, which could be mutated by callers!", debugName, description);
190190
}
191191

192-
// Log questionable members, but don't do the work if we're not going to log it
193-
if (LOG.isDebugEnabled())
194-
{
195-
analyzeValue(value, debugName, null, 1);
196-
}
192+
// Flag values that hold a Container or User object
193+
analyzeValue(value, debugName, null, 1);
197194
}
198195

199196
private static final int MAX_DEPTH = 4;
@@ -236,9 +233,7 @@ else if (CLASSES.add(clazz))
236233

237234
if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type))
238235
{
239-
// String message = cacheName + ": " + clazz.getName() + " field " + newFieldPath + " (" + field.getType().getName() + ")";
240-
// throw new IllegalStateException(message);
241-
LOG.debug("{}: {} field {} ({})", cacheName, clazz.getName(), newFieldPath, field.getType().getName());
236+
LOG.error("Cached value holds an unsafe object - {}: {} field {} ({})", cacheName, clazz.getName(), newFieldPath, field.getType().getName());
242237
}
243238
else
244239
{

‎api/src/org/labkey/api/security/ElevatedUser.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import org.labkey.api.audit.permissions.CanSeeAuditLogPermission;
44
import org.labkey.api.data.Container;
5+
import org.labkey.api.security.impersonation.ImpersonationContext;
56
import org.labkey.api.security.impersonation.WrappedImpersonationContext;
67
import org.labkey.api.security.permissions.Permission;
78
import org.labkey.api.security.roles.CanSeeAuditLogRole;
@@ -26,6 +27,11 @@ private ElevatedUser(User user, Set<Role> rolesToAdd)
2627
super(user, new WrappedImpersonationContext(user.getImpersonationContext(), rolesToAdd));
2728
}
2829

30+
private ElevatedUser(User user, ImpersonationContext ctx)
31+
{
32+
super(user, ctx);
33+
}
34+
2935
/**
3036
* Wrap the supplied user and unconditionally add the supplied role(s). Always returns an ElevatedUser.
3137
*/
@@ -43,6 +49,14 @@ public static ElevatedUser getElevatedUser(User user, Collection<Class<? extends
4349
return new ElevatedUser(user, getRoles(roleClassesToAdd));
4450
}
4551

52+
/**
53+
* Used to reconstitute an ElevatedUser from its component parts
54+
*/
55+
public static ElevatedUser getElevatedUser(User user, ImpersonationContext ctx)
56+
{
57+
return new ElevatedUser(user, ctx);
58+
}
59+
4660
/**
4761
* Ensure the supplied user has the supplied permissions. If so, return that user. If not, wrap the user with
4862
* ElevatedUser and, for each pair of permission + role, add the role if the user doesn't have the corresponding

‎assay/api-src/org/labkey/api/assay/dilution/DilutionAssayRun.java‎

Lines changed: 47 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,16 @@
1616
package org.labkey.api.assay.dilution;
1717

1818
import org.jetbrains.annotations.Nullable;
19+
import org.labkey.api.assay.AbstractAssayProvider;
20+
import org.labkey.api.assay.AssayProtocolSchema;
21+
import org.labkey.api.assay.AssayService;
1922
import org.labkey.api.assay.dilution.query.DilutionProviderSchema;
2023
import org.labkey.api.assay.nab.Luc5Assay;
2124
import org.labkey.api.assay.nab.NabGraph;
2225
import org.labkey.api.assay.nab.NabSpecimen;
2326
import org.labkey.api.assay.nab.RenderAssayBean;
2427
import org.labkey.api.assay.nab.view.RunDetailOptions;
28+
import org.labkey.api.assay.plate.WellGroup;
2529
import org.labkey.api.collections.CaseInsensitiveHashMap;
2630
import org.labkey.api.data.ColumnInfo;
2731
import org.labkey.api.data.Container;
@@ -42,11 +46,10 @@
4246
import org.labkey.api.query.CustomView;
4347
import org.labkey.api.query.FieldKey;
4448
import org.labkey.api.query.QueryService;
49+
import org.labkey.api.security.ElevatedUser;
4550
import org.labkey.api.security.User;
46-
import org.labkey.api.assay.plate.WellGroup;
47-
import org.labkey.api.assay.AbstractAssayProvider;
48-
import org.labkey.api.assay.AssayProtocolSchema;
49-
import org.labkey.api.assay.AssayService;
51+
import org.labkey.api.security.UserManager;
52+
import org.labkey.api.security.impersonation.ImpersonationContext;
5053
import org.labkey.api.view.NotFoundException;
5154
import org.labkey.api.view.ViewContext;
5255

@@ -64,35 +67,43 @@
6467
import java.util.Set;
6568
import java.util.TreeMap;
6669

67-
/**
68-
* User: klum
69-
* Date: 5/8/13
70-
*/
7170
public abstract class DilutionAssayRun extends Luc5Assay
7271
{
7372
protected ExpProtocol _protocol;
74-
protected DilutionAssayProvider _provider;
73+
protected DilutionAssayProvider<?> _provider;
7574
protected Map<PropertyDescriptor, Object> _runProperties;
7675
protected Map<PropertyDescriptor, Object> _runDisplayProperties;
7776
protected List<SampleResult> _sampleResults;
7877
protected Map<String, Object> _virusNames;
7978
protected ExpRun _run;
80-
// Be extremely careful to not leak this user out in any objects (e.g, via schemas or tables) as it may have elevated permissions.
81-
protected User _user;
8279
protected StatsService.CurveFitType _savedCurveFitType = null;
8380
protected Map<ExpMaterial, List<WellGroup>> _materialWellGroupMapping;
8481
protected Map<WellGroup, ExpMaterial> _wellGroupMaterialMapping;
8582

86-
public DilutionAssayRun(DilutionAssayProvider provider, ExpRun run,
83+
// Objects of this class are cached (held by NAbRunWrapper), so we don't want to hold onto a User object. The
84+
// passed in user might have elevated permissions, so stash the impersonation context along with the user ID.
85+
private final int _userId;
86+
private final ImpersonationContext _impersonationContext;
87+
private final Map<FieldKey, PropertyDescriptor> _fieldKeys;
88+
89+
public DilutionAssayRun(DilutionAssayProvider<?> provider, ExpRun run,
8790
User user, List<Integer> cutoffs, StatsService.CurveFitType renderCurveFitType)
8891
{
8992
super(run.getRowId(), cutoffs, renderCurveFitType);
9093
_run = run;
91-
_user = user;
94+
_userId = user.getUserId();
95+
_impersonationContext = user instanceof ElevatedUser eu ? eu.getImpersonationContext() : null;
9296
_protocol = run.getProtocol();
9397
_provider = provider;
98+
_fieldKeys = getFieldKeys(user);
9499

95-
for (Map.Entry<PropertyDescriptor, Object> property : getRunProperties().entrySet())
100+
TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, user, _run.getContainer(), null);
101+
Map<FieldKey, ColumnInfo> cols = QueryService.get().getColumns(runTable, _fieldKeys.keySet());
102+
Map<PropertyDescriptor, Object> runProperties = new TreeMap<>(new PropertyDescriptorComparator());
103+
runProperties.putAll(getRunProperties(runTable, _fieldKeys, cols));
104+
_runProperties = Collections.unmodifiableMap(runProperties);
105+
106+
for (Map.Entry<PropertyDescriptor, Object> property : _runProperties.entrySet())
96107
{
97108
if (DilutionAssayProvider.CURVE_FIT_METHOD_PROPERTY_NAME.equals(property.getKey().getName()))
98109
{
@@ -102,7 +113,13 @@ public DilutionAssayRun(DilutionAssayProvider provider, ExpRun run,
102113
}
103114
}
104115

105-
public DilutionAssayProvider getProvider()
116+
protected User getUser()
117+
{
118+
User user = UserManager.getUser(_userId);
119+
return _impersonationContext != null ? ElevatedUser.getElevatedUser(user, _impersonationContext) : user;
120+
}
121+
122+
public DilutionAssayProvider<?> getProvider()
106123
{
107124
return _provider;
108125
}
@@ -119,6 +136,11 @@ public String getRunName()
119136
}
120137

121138
private Map<FieldKey, PropertyDescriptor> getFieldKeys()
139+
{
140+
return _fieldKeys;
141+
}
142+
143+
private Map<FieldKey, PropertyDescriptor> getFieldKeys(User user)
122144
{
123145
Map<FieldKey, PropertyDescriptor> fieldKeys = new HashMap<>();
124146
for (DomainProperty property : _provider.getBatchDomain(_protocol).getProperties())
@@ -127,7 +149,7 @@ private Map<FieldKey, PropertyDescriptor> getFieldKeys()
127149
fieldKeys.put(FieldKey.fromParts(property.getName()), property.getPropertyDescriptor());
128150

129151
// Add all of the hard columns to the set of properties we can show
130-
TableInfo runTableInfo = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null);
152+
TableInfo runTableInfo = AssayService.get().createRunTable(_protocol, _provider, user, _run.getContainer(), null);
131153
for (ColumnInfo runColumn : runTableInfo.getColumns())
132154
{
133155
// These columns cause an UnauthorizedException if the user has permission to see the dataset
@@ -149,7 +171,7 @@ private Map<FieldKey, PropertyDescriptor> getFieldKeys()
149171
}
150172
}
151173

152-
return fieldKeys;
174+
return Collections.unmodifiableMap(fieldKeys);
153175
}
154176

155177
public StatsService.CurveFitType getSavedCurveFitType()
@@ -196,7 +218,7 @@ public Map<PropertyDescriptor, Object> getRunDisplayProperties(ViewContext conte
196218
if (_runDisplayProperties == null)
197219
{
198220
Map<FieldKey, PropertyDescriptor> fieldKeys = getFieldKeys();
199-
TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null);
221+
TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, getUser(), _run.getContainer(), null);
200222

201223
CustomView runView = getRunsCustomView(context);
202224
Collection<FieldKey> fieldKeysToShow;
@@ -211,7 +233,7 @@ public Map<PropertyDescriptor, Object> getRunDisplayProperties(ViewContext conte
211233
fieldKeysToShow = new ArrayList<>(runTable.getDefaultVisibleColumns());
212234
}
213235
// The list of available columns is reduced from the default set because the user may not have
214-
// permission to join to all of the lookups. Remove any columns that aren't part of the acceptable set,
236+
// permission to join to all the lookups. Remove any columns that aren't part of the acceptable set,
215237
// which is built up by getFieldKeys()
216238
List<FieldKey> newFieldKeysToShow = new ArrayList<>();
217239
for (FieldKey fieldKey : fieldKeysToShow)
@@ -229,9 +251,9 @@ public Map<PropertyDescriptor, Object> getRunDisplayProperties(ViewContext conte
229251
}
230252

231253
Map<FieldKey, ColumnInfo> selectCols = QueryService.get().getColumns(runTable, newFieldKeysToShow);
232-
_runDisplayProperties = getRunProperties(runTable, fieldKeys, selectCols);
254+
_runDisplayProperties = Collections.unmodifiableMap(getRunProperties(runTable, fieldKeys, selectCols));
233255
}
234-
return Collections.unmodifiableMap(_runDisplayProperties);
256+
return _runDisplayProperties;
235257
}
236258

237259
protected Map<String, Map<PropertyDescriptor, Object>> getSampleProperties()
@@ -256,7 +278,7 @@ protected Map<String, DilutionResultProperties> getSampleProperties(ExpData outp
256278
{
257279
Map<String, DilutionResultProperties> dilutionResultPropertiesMap = new HashMap<>();
258280

259-
AssayProtocolSchema schema = _provider.createProtocolSchema(_user, _run.getContainer(), _protocol, null);
281+
AssayProtocolSchema schema = _provider.createProtocolSchema(getUser(), _run.getContainer(), _protocol, null);
260282
TableInfo virusTable = schema.createTable(DilutionManager.VIRUS_TABLE_NAME, null);
261283

262284
// Do a query to get all the info we need to do the linkage
@@ -316,23 +338,15 @@ protected CustomView getRunsCustomView(ViewContext context)
316338

317339
public Map<PropertyDescriptor, Object> getRunProperties()
318340
{
319-
if (_runProperties == null)
320-
{
321-
Map<FieldKey, PropertyDescriptor> fieldKeys = getFieldKeys();
322-
TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null);
323-
Map<FieldKey, ColumnInfo> cols = QueryService.get().getColumns(runTable, fieldKeys.keySet());
324-
_runProperties = new TreeMap<>(new PropertyDescriptorComparator());
325-
_runProperties.putAll(getRunProperties(runTable, fieldKeys, cols));
326-
}
327-
return Collections.unmodifiableMap(_runProperties);
341+
return _runProperties;
328342
}
329343

330344
public abstract List<SampleResult> getSampleResults();
331345

332346
public Map<String, Object> getVirusNames()
333347
{
334348
if (_virusNames == null)
335-
_virusNames = Collections.EMPTY_MAP;
349+
_virusNames = Collections.emptyMap();
336350
return _virusNames;
337351
}
338352

@@ -377,7 +391,7 @@ public static class SampleResult
377391
private boolean _longCaptions = false;
378392
private final DilutionManager _mgr = new DilutionManager();
379393

380-
public SampleResult(DilutionAssayProvider provider, ExpData data, DilutionSummary dilutionSummary, DilutionMaterialKey materialKey,
394+
public SampleResult(DilutionAssayProvider<?> provider, ExpData data, DilutionSummary dilutionSummary, DilutionMaterialKey materialKey,
381395
Map<PropertyDescriptor, Object> sampleProperties, DilutionResultProperties dilutionResultProperties)
382396
{
383397
_dilutionSummary = dilutionSummary;

‎issues/src/org/labkey/issue/model/IssueListDef.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.labkey.api.security.Group;
4141
import org.labkey.api.security.SecurityManager;
4242
import org.labkey.api.security.User;
43+
import org.labkey.api.util.GUID;
4344
import org.labkey.api.util.UnexpectedException;
4445
import org.labkey.issue.query.IssueDefDomainKind;
4546

@@ -55,7 +56,7 @@ public class IssueListDef extends Entity
5556
private String _name;
5657
private String _label;
5758
private String _kind;
58-
private Container _domainContainer;
59+
private GUID _domainContainerId;
5960

6061
public int getRowId()
6162
{
@@ -111,7 +112,7 @@ public TableInfo createTable(User user)
111112
@Nullable
112113
public Container getDomainContainer(User user)
113114
{
114-
if (_domainContainer == null)
115+
if (_domainContainerId == null)
115116
{
116117
String id = getContainerId();
117118
if (id != null)
@@ -125,16 +126,16 @@ public Container getDomainContainer(User user)
125126
// create the domain in the current container
126127
if (domain != null)
127128
{
128-
_domainContainer = domain.getContainer();
129+
_domainContainerId = domain.getContainer().getEntityId();
129130
}
130131
else
131132
{
132-
_domainContainer = container;
133+
_domainContainerId = container.getEntityId();
133134
}
134135
}
135136
}
136137
}
137-
return _domainContainer;
138+
return ContainerManager.getForId(_domainContainerId);
138139
}
139140

140141
public Domain getDomain(User user)

0 commit comments

Comments
 (0)