diff --git a/api/src/org/labkey/api/search/SearchService.java b/api/src/org/labkey/api/search/SearchService.java index 874b22cbe3f..f8cfce69be8 100644 --- a/api/src/org/labkey/api/search/SearchService.java +++ b/api/src/org/labkey/api/search/SearchService.java @@ -305,19 +305,14 @@ public String toString() return _name; } - protected Set getPermittedContainerIds(User user, Map containers, @NotNull Class perm) - { - Set containerIds = new HashSet<>(); - containers.forEach((id, container) -> { - if (container.hasPermission(user, perm)) - containerIds.add(id); - }); - return containerIds.size() == containers.size() ? containers.keySet() : containerIds; - } - - public Set getPermittedContainerIds(User user, Map containers) + /** + * Permission required, beyond base container Read (which every searchable container already has), for this + * category's documents to be visible. Return null if base Read is sufficient. + */ + @Nullable + public Class getRequiredPermission() { - return containers.keySet(); + return null; } public boolean isShowInAdvancedSearch() diff --git a/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java b/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java index 7852356db7a..f823b2bf5f2 100644 --- a/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java +++ b/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java @@ -38,6 +38,7 @@ public class MultiPhaseCPUTimer> private final K[] _values; private long _count = 0; + private boolean _clearedFirstInvocation = false; public MultiPhaseCPUTimer(Class clazz, K[] values) { @@ -87,6 +88,27 @@ public Map getTimes() return map; } + public void clearTimes() + { + synchronized (_accumulationMap) + { + _accumulationMap.values().forEach(v -> v.setValue(0)); + _count = 0; + } + } + + public void clearTimesIfFirstInvocation() + { + synchronized (_accumulationMap) + { + if (!_clearedFirstInvocation) + { + _clearedFirstInvocation = true; + clearTimes(); + } + } + } + // Create an enum map and populate it with MutableLongs for each value private static > Map getEnumMap(Class clazz, ENUM[] values) { diff --git a/assay/src/org/labkey/assay/AssayManager.java b/assay/src/org/labkey/assay/AssayManager.java index 9219ae1f5c8..9d3eb6ba5a2 100644 --- a/assay/src/org/labkey/assay/AssayManager.java +++ b/assay/src/org/labkey/assay/AssayManager.java @@ -76,6 +76,7 @@ import org.labkey.api.security.User; import org.labkey.api.security.permissions.AssayReadPermission; import org.labkey.api.security.permissions.InsertPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.settings.AppProps; import org.labkey.api.study.assay.ParticipantVisitResolver; import org.labkey.api.study.assay.ParticipantVisitResolverType; @@ -120,23 +121,23 @@ public class AssayManager implements AssayService { SearchService.SearchCategory ASSAY_CATEGORY = new SearchService.SearchCategory("assay", "Assays") { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, AssayReadPermission.class); + return AssayReadPermission.class; } }; SearchService.SearchCategory ASSAY_BATCH_CATEGORY = new SearchService.SearchCategory("assayBatch", "Assay Batches", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, AssayReadPermission.class); + return AssayReadPermission.class; } }; SearchService.SearchCategory ASSAY_RUN_CATEGORY = new SearchService.SearchCategory("assayRun", "Assay Runs", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, AssayReadPermission.class); + return AssayReadPermission.class; } }; diff --git a/assay/src/org/labkey/assay/plate/PlateManager.java b/assay/src/org/labkey/assay/plate/PlateManager.java index 39c29f1c289..a26f8e661b5 100644 --- a/assay/src/org/labkey/assay/plate/PlateManager.java +++ b/assay/src/org/labkey/assay/plate/PlateManager.java @@ -116,6 +116,7 @@ import org.labkey.api.query.ValidationException; import org.labkey.api.reader.ColumnDescriptor; import org.labkey.api.search.SearchService; +import org.labkey.api.search.SearchService.SearchCategory; import org.labkey.api.security.User; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; @@ -220,21 +221,8 @@ public class PlateManager implements PlateService, AssayListener, ExperimentList // when those calls are being made for a plate save operation. public static final String PLATE_SAVE_FLAG = ".plateSave"; - public SearchService.SearchCategory PLATE_CATEGORY = new SearchService.SearchCategory("plate", "Assay Plates", false) { - @Override - public Set getPermittedContainerIds(User user, Map containers) - { - return getPermittedContainerIds(user, containers, ReadPermission.class); - } - }; - - public SearchService.SearchCategory PLATE_SET_CATEGORY = new SearchService.SearchCategory("plateSet", "Assay Plate Sets", false) { - @Override - public Set getPermittedContainerIds(User user, Map containers) - { - return getPermittedContainerIds(user, containers, ReadPermission.class); - } - }; + public SearchCategory PLATE_CATEGORY = new SearchCategory("plate", "Assay Plates", false); + public SearchCategory PLATE_SET_CATEGORY = new SearchCategory("plateSet", "Assay Plate Sets", false); public static PlateManager get() { diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java index b1984980873..c19d2456587 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java @@ -48,6 +48,7 @@ import org.labkey.api.security.User; import org.labkey.api.security.permissions.DataClassReadPermission; import org.labkey.api.security.permissions.MediaReadPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; import org.labkey.api.util.UnexpectedException; @@ -76,16 +77,16 @@ public class ExpDataClassImpl extends ExpIdentifiableEntityImpl imple private static final String MEDIA_SEARCH_CATEGORY_NAME = "media"; public static final SearchService.SearchCategory SEARCH_CATEGORY = new SearchService.SearchCategory(SEARCH_CATEGORY_NAME, "Collections of data objects", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, DataClassReadPermission.class); + return DataClassReadPermission.class; } }; public static final SearchService.SearchCategory MEDIA_SEARCH_CATEGORY = new SearchService.SearchCategory(MEDIA_SEARCH_CATEGORY_NAME, "Collections of media data and samples", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, MediaReadPermission.class); + return MediaReadPermission.class; } }; diff --git a/experiment/src/org/labkey/experiment/api/ExpDataImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataImpl.java index eb38273ddc2..b8306ad25cc 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataImpl.java @@ -133,16 +133,16 @@ public Class getPermissionClass() public static final SearchService.SearchCategory expDataCategory = new SearchService.SearchCategory("data", "ExpData", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, DataClassReadPermission.class); + return DataClassReadPermission.class; } }; public static final SearchService.SearchCategory expMediaDataCategory = new SearchService.SearchCategory("mediaData", "ExpData for media objects", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, MediaReadPermission.class); + return MediaReadPermission.class; } }; diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java index 152dd004add..192683c66fc 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java @@ -59,6 +59,7 @@ import org.labkey.api.search.SearchService; import org.labkey.api.security.User; import org.labkey.api.security.permissions.MediaReadPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.study.StudyService; import org.labkey.api.util.JobRunner; import org.labkey.api.util.PageFlowUtil; @@ -88,9 +89,9 @@ public class ExpMaterialImpl extends AbstractRunItemImpl implements Ex public static final SearchService.SearchCategory searchCategory = new SearchService.SearchCategory("material", "Materials/Samples", false); public static final SearchService.SearchCategory mediaSearchCategory = new SearchService.SearchCategory("media", "Media Samples", false){ @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, MediaReadPermission.class); + return MediaReadPermission.class; } }; diff --git a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java index 1638e121f84..36acf398e88 100644 --- a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java @@ -63,6 +63,7 @@ import org.labkey.api.search.SearchService; import org.labkey.api.security.User; import org.labkey.api.security.permissions.MediaReadPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.study.StudyService; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; @@ -97,9 +98,9 @@ public class ExpSampleTypeImpl extends ExpIdentifiableEntityImpl public static final SearchService.SearchCategory searchCategory = new SearchService.SearchCategory(categoryName, "Sample Types", false); public static final SearchService.SearchCategory mediaSearchCategory = new SearchService.SearchCategory(mediaCategoryName, "Media Sample Types", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, MediaReadPermission.class); + return MediaReadPermission.class; } }; diff --git a/search/src/org/labkey/search/SearchModule.java b/search/src/org/labkey/search/SearchModule.java index 69e652a08a1..529e7abdfb9 100644 --- a/search/src/org/labkey/search/SearchModule.java +++ b/search/src/org/labkey/search/SearchModule.java @@ -58,6 +58,7 @@ import org.labkey.search.model.PlainTextDocumentParser; import org.labkey.search.model.SearchSchema; import org.labkey.search.model.SearchStartupProperties; +import org.labkey.search.model.SecurityQuery; import org.labkey.search.view.SearchWebPartFactory; import javax.management.StandardMBean; @@ -259,7 +260,7 @@ private void reindexIfNeeded(@NotNull SearchService ss) @Override public @NotNull Set> getUnitTests() { - return Set.of(AbstractSearchService.TestCase.class); + return Set.of(AbstractSearchService.TestCase.class, SecurityQuery.TestCase.class); } @Override diff --git a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java index 1350942d544..48363cbe993 100644 --- a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java +++ b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java @@ -1824,6 +1824,7 @@ else if (options.sortField.equals(FIELD_NAME.container.name())) finally { TIMER.releaseInvocationTimer(iTimer); + TIMER.clearTimesIfFirstInvocation(); // Toss the very first invocation since it likely had to warm the caches, etc. } } diff --git a/search/src/org/labkey/search/model/SecurityQuery.java b/search/src/org/labkey/search/model/SecurityQuery.java index 074fda60af5..aa9ba86d32b 100644 --- a/search/src/org/labkey/search/model/SecurityQuery.java +++ b/search/src/org/labkey/search/model/SecurityQuery.java @@ -16,6 +16,8 @@ package org.labkey.search.model; +import org.apache.commons.collections4.MultiValuedMap; +import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.lang3.StringUtils; import org.apache.lucene.index.BinaryDocValues; import org.apache.lucene.index.LeafReader; @@ -34,25 +36,35 @@ import org.apache.lucene.util.FixedBitSet; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.module.Module; import org.labkey.api.search.SearchScope; import org.labkey.api.search.SearchService; +import org.labkey.api.search.SearchService.SearchCategory; import org.labkey.api.security.SecurableResource; +import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.DeletePermission; +import org.labkey.api.security.permissions.InsertPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.util.MultiPhaseCPUTimer.InvocationTimer; import org.labkey.search.model.LuceneSearchServiceImpl.FIELD_NAME; import java.io.IOException; +import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import static org.apache.lucene.search.DocIdSetIterator.NO_MORE_DOCS; -class SecurityQuery extends Query +public class SecurityQuery extends Query { private final User _user; private final Container _currentContainer; @@ -75,11 +87,73 @@ class SecurityQuery extends Query _containerIds = searchScope.getSearchableContainers(user, currentContainer); - SearchService.get().getSearchCategories().forEach( - category -> { - _categoryContainers.put(category.getName(), category.getPermittedContainerIds(user, _containerIds)); - } - ); + // Categories that require only base container Read (already guaranteed for every container above) are + // resolved directly; the rest are grouped by required permission so multiple categories that require the + // same permission (e.g., the three assay categories all require AssayReadPermission) share a single + // O(containers) assembly pass below instead of each redoing it. + CategoryPermissions categoryPermissions = groupCategoriesByRequiredPermission(SearchService.get().getSearchCategories()); + + for (String categoryName : categoryPermissions.baseReadCategoryNames()) + _categoryContainers.put(categoryName, _containerIds.keySet()); + + // Containers that inherit their policy (e.g., workbooks, which typically don't have their own explicit + // policy) share the exact same SecurityPolicy object as their nearest ancestor with one. Role resolution + // (SecurityManager.getPermissions()) is therefore identical for every container backed by the same policy, + // so compute it once per distinct policy instead of once per container per category. A user's full granted + // permission set can be large (100+ for a site admin), but categories only ever ask about a handful of + // permission classes, so retain just those instead of holding the full set for every distinct policy. + Map, Collection> categoriesByPermission = categoryPermissions.categoriesByPermission(); + Set> requiredPermissions = categoriesByPermission.keySet(); + HashMap>> permissionsByPolicy = new HashMap<>(); + + if (!requiredPermissions.isEmpty()) + { + for (Container c : _containerIds.values()) + { + permissionsByPolicy.computeIfAbsent(c.getPolicy().getResourceId(), _ -> { + Set> permitted = new HashSet<>(requiredPermissions); + permitted.retainAll(SecurityManager.getPermissions(c, user, null)); + return permitted; + }); + } + } + + categoriesByPermission.forEach((requiredPermission, categories) -> { + Set permittedContainerIds = new HashSet<>(); + + for (var entry : _containerIds.entrySet()) + { + if (permissionsByPolicy.get(entry.getValue().getPolicy().getResourceId()).contains(requiredPermission)) + permittedContainerIds.add(entry.getKey()); + } + + for (SearchCategory category : categories) + _categoryContainers.put(category.getName(), permittedContainerIds); + }); + } + + record CategoryPermissions(Map, Collection> categoriesByPermission, Set baseReadCategoryNames){} + + /** + * Splits categories into those requiring only base container Read (their names are added to baseReadCategoryNames) + * and those requiring a specific permission, which are grouped by that permission class. + */ + static CategoryPermissions groupCategoriesByRequiredPermission(Collection categories) + { + MultiValuedMap, SearchCategory> categoriesByPermission = new ArrayListValuedHashMap<>(); + Set baseReadCategoryNames = new HashSet<>(); + + for (SearchCategory category : categories) + { + Class requiredPermission = category.getRequiredPermission(); + + if (null == requiredPermission) + baseReadCategoryNames.add(category.getName()); + else + categoriesByPermission.put(requiredPermission, category); + } + + return new CategoryPermissions(categoriesByPermission.asMap(), baseReadCategoryNames); } @Override @@ -314,4 +388,70 @@ public boolean mayInheritPolicy() return false; } } + + public static class TestCase extends Assert + { + private static SearchCategory categoryRequiring(String name, Class requiredPermission) + { + return new SearchCategory(name, name, false) + { + @Override + public Class getRequiredPermission() + { + return requiredPermission; + } + }; + } + + @Test + public void testCategoryWithNoRequiredPermissionGoesToBaseRead() + { + SearchCategory wiki = new SearchCategory("wiki", "Wiki Pages"); + + CategoryPermissions result = SecurityQuery.groupCategoriesByRequiredPermission(List.of(wiki)); + + assertEquals(Set.of("wiki"), result.baseReadCategoryNames()); + assertTrue(result.categoriesByPermission().isEmpty()); + } + + @Test + public void testCategoriesSharingAPermissionAreGroupedTogether() + { + // Mirrors the real assay/assayBatch/assayRun categories, which all require the same permission. + SearchCategory assay = categoryRequiring("assay", InsertPermission.class); + SearchCategory assayBatch = categoryRequiring("assayBatch", InsertPermission.class); + SearchCategory assayRun = categoryRequiring("assayRun", InsertPermission.class); + + CategoryPermissions result = SecurityQuery.groupCategoriesByRequiredPermission(List.of(assay, assayBatch, assayRun)); + + assertTrue(result.baseReadCategoryNames().isEmpty()); + assertEquals(Set.of(InsertPermission.class), result.categoriesByPermission().keySet()); + assertEquals(Set.of(assay, assayBatch, assayRun), Set.copyOf(result.categoriesByPermission().get(InsertPermission.class))); + } + + @Test + public void testCategoriesWithDifferentPermissionsAreNotGroupedTogether() + { + SearchCategory data = categoryRequiring("data", InsertPermission.class); + SearchCategory media = categoryRequiring("media", DeletePermission.class); + + CategoryPermissions result = SecurityQuery.groupCategoriesByRequiredPermission(List.of(data, media)); + + assertEquals(Set.of(InsertPermission.class, DeletePermission.class), result.categoriesByPermission().keySet()); + assertEquals(List.of(data), result.categoriesByPermission().get(InsertPermission.class)); + assertEquals(List.of(media), result.categoriesByPermission().get(DeletePermission.class)); + } + + @Test + public void testMixOfBaseReadAndPermissionRequiringCategories() + { + SearchCategory wiki = new SearchCategory("wiki", "Wiki Pages"); + SearchCategory data = categoryRequiring("data", InsertPermission.class); + + CategoryPermissions result = SecurityQuery.groupCategoriesByRequiredPermission(List.of(wiki, data)); + + assertEquals(Set.of("wiki"), result.baseReadCategoryNames()); + assertEquals(List.of(data), result.categoriesByPermission().get(InsertPermission.class)); + } + } }