diff --git a/api/src/org/labkey/api/cache/CacheManager.java b/api/src/org/labkey/api/cache/CacheManager.java index f5b647bbe8c..1ff35129b1f 100644 --- a/api/src/org/labkey/api/cache/CacheManager.java +++ b/api/src/org/labkey/api/cache/CacheManager.java @@ -60,7 +60,7 @@ public class CacheManager public static final int DEFAULT_CACHE_SIZE = 5000; // Set useCache = false to completely disable all caching... and slow your server to a near halt. Possibly useful for - // reproducing CacheLoader re-entrancy problems, but not much else. + // reproducing CacheLoader reentrancy problems, but not much else. private static final boolean useCache = true; private static final CacheProvider PROVIDER = useCache ? EhCacheProvider.getInstance() : new NoopCacheProvider(); @@ -176,7 +176,7 @@ public static void shutdown() private static final Set> CLASSES = new HashSet<>(); - // Validate a cached value. For now, just log warnings for mutable collections. + // Validate a cached value. Log errors for mutable collections/arrays and for values holding Container or User objects. public static void validate(String debugName, @Nullable V value) { if (value instanceof Wrapper) @@ -186,14 +186,11 @@ public static void validate(String debugName, @Nullable V value) if (null != description) { - LOG.warn("{} attempted to cache {}, which could be mutated by callers!", debugName, description); + LOG.error("{} attempted to cache {}, which could be mutated by callers!", debugName, description); } - // Log questionable members, but don't do the work if we're not going to log it - if (LOG.isDebugEnabled()) - { - analyzeValue(value, debugName, null, 1); - } + // Flag values that hold a Container or User object + analyzeValue(value, debugName, null, 1); } private static final int MAX_DEPTH = 4; @@ -236,9 +233,7 @@ else if (CLASSES.add(clazz)) if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type)) { -// String message = cacheName + ": " + clazz.getName() + " field " + newFieldPath + " (" + field.getType().getName() + ")"; -// throw new IllegalStateException(message); - LOG.debug("{}: {} field {} ({})", cacheName, clazz.getName(), newFieldPath, field.getType().getName()); + LOG.error("Cached value holds an unsafe object - {}: {} field {} ({})", cacheName, clazz.getName(), newFieldPath, field.getType().getName()); } else { diff --git a/api/src/org/labkey/api/security/ElevatedUser.java b/api/src/org/labkey/api/security/ElevatedUser.java index 0aefd33f4ba..128cafef712 100644 --- a/api/src/org/labkey/api/security/ElevatedUser.java +++ b/api/src/org/labkey/api/security/ElevatedUser.java @@ -2,6 +2,7 @@ import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.data.Container; +import org.labkey.api.security.impersonation.ImpersonationContext; import org.labkey.api.security.impersonation.WrappedImpersonationContext; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.roles.CanSeeAuditLogRole; @@ -26,6 +27,11 @@ private ElevatedUser(User user, Set rolesToAdd) super(user, new WrappedImpersonationContext(user.getImpersonationContext(), rolesToAdd)); } + private ElevatedUser(User user, ImpersonationContext ctx) + { + super(user, ctx); + } + /** * Wrap the supplied user and unconditionally add the supplied role(s). Always returns an ElevatedUser. */ @@ -43,6 +49,14 @@ public static ElevatedUser getElevatedUser(User user, Collection _provider; protected Map _runProperties; protected Map _runDisplayProperties; protected List _sampleResults; protected Map _virusNames; protected ExpRun _run; - // Be extremely careful to not leak this user out in any objects (e.g, via schemas or tables) as it may have elevated permissions. - protected User _user; protected StatsService.CurveFitType _savedCurveFitType = null; protected Map> _materialWellGroupMapping; protected Map _wellGroupMaterialMapping; - public DilutionAssayRun(DilutionAssayProvider provider, ExpRun run, + // Objects of this class are cached (held by NAbRunWrapper), so we don't want to hold onto a User object. The + // passed in user might have elevated permissions, so stash the impersonation context along with the user ID. + private final int _userId; + private final ImpersonationContext _impersonationContext; + private final Map _fieldKeys; + + public DilutionAssayRun(DilutionAssayProvider provider, ExpRun run, User user, List cutoffs, StatsService.CurveFitType renderCurveFitType) { super(run.getRowId(), cutoffs, renderCurveFitType); _run = run; - _user = user; + _userId = user.getUserId(); + _impersonationContext = user instanceof ElevatedUser eu ? eu.getImpersonationContext() : null; _protocol = run.getProtocol(); _provider = provider; + _fieldKeys = getFieldKeys(user); - for (Map.Entry property : getRunProperties().entrySet()) + TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, user, _run.getContainer(), null); + Map cols = QueryService.get().getColumns(runTable, _fieldKeys.keySet()); + Map runProperties = new TreeMap<>(new PropertyDescriptorComparator()); + runProperties.putAll(getRunProperties(runTable, _fieldKeys, cols)); + _runProperties = Collections.unmodifiableMap(runProperties); + + for (Map.Entry property : _runProperties.entrySet()) { if (DilutionAssayProvider.CURVE_FIT_METHOD_PROPERTY_NAME.equals(property.getKey().getName())) { @@ -102,7 +113,13 @@ public DilutionAssayRun(DilutionAssayProvider provider, ExpRun run, } } - public DilutionAssayProvider getProvider() + protected User getUser() + { + User user = UserManager.getUser(_userId); + return _impersonationContext != null ? ElevatedUser.getElevatedUser(user, _impersonationContext) : user; + } + + public DilutionAssayProvider getProvider() { return _provider; } @@ -119,6 +136,11 @@ public String getRunName() } private Map getFieldKeys() + { + return _fieldKeys; + } + + private Map getFieldKeys(User user) { Map fieldKeys = new HashMap<>(); for (DomainProperty property : _provider.getBatchDomain(_protocol).getProperties()) @@ -127,7 +149,7 @@ private Map getFieldKeys() fieldKeys.put(FieldKey.fromParts(property.getName()), property.getPropertyDescriptor()); // Add all of the hard columns to the set of properties we can show - TableInfo runTableInfo = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null); + TableInfo runTableInfo = AssayService.get().createRunTable(_protocol, _provider, user, _run.getContainer(), null); for (ColumnInfo runColumn : runTableInfo.getColumns()) { // These columns cause an UnauthorizedException if the user has permission to see the dataset @@ -149,7 +171,7 @@ private Map getFieldKeys() } } - return fieldKeys; + return Collections.unmodifiableMap(fieldKeys); } public StatsService.CurveFitType getSavedCurveFitType() @@ -196,7 +218,7 @@ public Map getRunDisplayProperties(ViewContext conte if (_runDisplayProperties == null) { Map fieldKeys = getFieldKeys(); - TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null); + TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, getUser(), _run.getContainer(), null); CustomView runView = getRunsCustomView(context); Collection fieldKeysToShow; @@ -211,7 +233,7 @@ public Map getRunDisplayProperties(ViewContext conte fieldKeysToShow = new ArrayList<>(runTable.getDefaultVisibleColumns()); } // The list of available columns is reduced from the default set because the user may not have - // permission to join to all of the lookups. Remove any columns that aren't part of the acceptable set, + // permission to join to all the lookups. Remove any columns that aren't part of the acceptable set, // which is built up by getFieldKeys() List newFieldKeysToShow = new ArrayList<>(); for (FieldKey fieldKey : fieldKeysToShow) @@ -229,9 +251,9 @@ public Map getRunDisplayProperties(ViewContext conte } Map selectCols = QueryService.get().getColumns(runTable, newFieldKeysToShow); - _runDisplayProperties = getRunProperties(runTable, fieldKeys, selectCols); + _runDisplayProperties = Collections.unmodifiableMap(getRunProperties(runTable, fieldKeys, selectCols)); } - return Collections.unmodifiableMap(_runDisplayProperties); + return _runDisplayProperties; } protected Map> getSampleProperties() @@ -256,7 +278,7 @@ protected Map getSampleProperties(ExpData outp { Map dilutionResultPropertiesMap = new HashMap<>(); - AssayProtocolSchema schema = _provider.createProtocolSchema(_user, _run.getContainer(), _protocol, null); + AssayProtocolSchema schema = _provider.createProtocolSchema(getUser(), _run.getContainer(), _protocol, null); TableInfo virusTable = schema.createTable(DilutionManager.VIRUS_TABLE_NAME, null); // Do a query to get all the info we need to do the linkage @@ -316,15 +338,7 @@ protected CustomView getRunsCustomView(ViewContext context) public Map getRunProperties() { - if (_runProperties == null) - { - Map fieldKeys = getFieldKeys(); - TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null); - Map cols = QueryService.get().getColumns(runTable, fieldKeys.keySet()); - _runProperties = new TreeMap<>(new PropertyDescriptorComparator()); - _runProperties.putAll(getRunProperties(runTable, fieldKeys, cols)); - } - return Collections.unmodifiableMap(_runProperties); + return _runProperties; } public abstract List getSampleResults(); @@ -332,7 +346,7 @@ public Map getRunProperties() public Map getVirusNames() { if (_virusNames == null) - _virusNames = Collections.EMPTY_MAP; + _virusNames = Collections.emptyMap(); return _virusNames; } @@ -377,7 +391,7 @@ public static class SampleResult private boolean _longCaptions = false; private final DilutionManager _mgr = new DilutionManager(); - public SampleResult(DilutionAssayProvider provider, ExpData data, DilutionSummary dilutionSummary, DilutionMaterialKey materialKey, + public SampleResult(DilutionAssayProvider provider, ExpData data, DilutionSummary dilutionSummary, DilutionMaterialKey materialKey, Map sampleProperties, DilutionResultProperties dilutionResultProperties) { _dilutionSummary = dilutionSummary; diff --git a/issues/src/org/labkey/issue/model/IssueListDef.java b/issues/src/org/labkey/issue/model/IssueListDef.java index 270f8acff01..1daec0f046a 100644 --- a/issues/src/org/labkey/issue/model/IssueListDef.java +++ b/issues/src/org/labkey/issue/model/IssueListDef.java @@ -40,6 +40,7 @@ import org.labkey.api.security.Group; import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; +import org.labkey.api.util.GUID; import org.labkey.api.util.UnexpectedException; import org.labkey.issue.query.IssueDefDomainKind; @@ -55,7 +56,7 @@ public class IssueListDef extends Entity private String _name; private String _label; private String _kind; - private Container _domainContainer; + private GUID _domainContainerId; public int getRowId() { @@ -111,7 +112,7 @@ public TableInfo createTable(User user) @Nullable public Container getDomainContainer(User user) { - if (_domainContainer == null) + if (_domainContainerId == null) { String id = getContainerId(); if (id != null) @@ -125,16 +126,16 @@ public Container getDomainContainer(User user) // create the domain in the current container if (domain != null) { - _domainContainer = domain.getContainer(); + _domainContainerId = domain.getContainer().getEntityId(); } else { - _domainContainer = container; + _domainContainerId = container.getEntityId(); } } } } - return _domainContainer; + return ContainerManager.getForId(_domainContainerId); } public Domain getDomain(User user)