diff --git a/api/src/org/labkey/api/search/SearchService.java b/api/src/org/labkey/api/search/SearchService.java index 874b22cbe3f..cc606b2faf7 100644 --- a/api/src/org/labkey/api/search/SearchService.java +++ b/api/src/org/labkey/api/search/SearchService.java @@ -187,7 +187,7 @@ public String toString() } } - enum SEARCH_PHASE {createQuery, buildSecurityFilter, search, applySecurityFilter, processHits} + enum SEARCH_PHASE {createQuery, buildSecurityFilterOld, buildSecurityFilter, search, applySecurityFilter, processHits} interface TaskListener { @@ -305,6 +305,17 @@ public String toString() return _name; } + /** + * 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 null; + } + + @Deprecated // TODO: Remove after testing protected Set getPermittedContainerIds(User user, Map containers, @NotNull Class perm) { Set containerIds = new HashSet<>(); @@ -315,6 +326,7 @@ protected Set getPermittedContainerIds(User user, Map return containerIds.size() == containers.size() ? containers.keySet() : containerIds; } + @Deprecated // TODO: Remove after testing public Set getPermittedContainerIds(User user, Map containers) { return containers.keySet(); 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..0e06ec69887 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; @@ -119,6 +120,12 @@ public class AssayManager implements AssayService { SearchService.SearchCategory ASSAY_CATEGORY = new SearchService.SearchCategory("assay", "Assays") { + @Override + public Class getRequiredPermission() + { + return AssayReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { @@ -126,6 +133,12 @@ public Set getPermittedContainerIds(User user, Map co } }; SearchService.SearchCategory ASSAY_BATCH_CATEGORY = new SearchService.SearchCategory("assayBatch", "Assay Batches", false) { + @Override + public Class getRequiredPermission() + { + return AssayReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { @@ -133,6 +146,12 @@ public Set getPermittedContainerIds(User user, Map co } }; SearchService.SearchCategory ASSAY_RUN_CATEGORY = new SearchService.SearchCategory("assayRun", "Assay Runs", false) { + @Override + public Class getRequiredPermission() + { + return AssayReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { diff --git a/assay/src/org/labkey/assay/plate/PlateManager.java b/assay/src/org/labkey/assay/plate/PlateManager.java index 39c29f1c289..9893c8e9fd7 100644 --- a/assay/src/org/labkey/assay/plate/PlateManager.java +++ b/assay/src/org/labkey/assay/plate/PlateManager.java @@ -220,6 +220,9 @@ 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"; + // No getRequiredPermission() override needed: base container Read (already required to reach this category's + // containers) is sufficient. The old getPermittedContainerIds() override below is kept temporarily for + // old-vs-new comparison testing even though it's a redundant re-check of Read permission. public SearchService.SearchCategory PLATE_CATEGORY = new SearchService.SearchCategory("plate", "Assay Plates", false) { @Override public Set getPermittedContainerIds(User user, Map containers) diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java index b1984980873..346cb035d6c 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; @@ -75,6 +76,12 @@ public class ExpDataClassImpl extends ExpIdentifiableEntityImpl imple private static final String SEARCH_CATEGORY_NAME = "dataClass"; 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 Class getRequiredPermission() + { + return DataClassReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { @@ -82,6 +89,12 @@ public Set getPermittedContainerIds(User user, Map co } }; public static final SearchService.SearchCategory MEDIA_SEARCH_CATEGORY = new SearchService.SearchCategory(MEDIA_SEARCH_CATEGORY_NAME, "Collections of media data and samples", false) { + @Override + public Class getRequiredPermission() + { + return MediaReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { diff --git a/experiment/src/org/labkey/experiment/api/ExpDataImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataImpl.java index eb38273ddc2..cd05e90a669 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataImpl.java @@ -132,6 +132,12 @@ public Class getPermissionClass() } public static final SearchService.SearchCategory expDataCategory = new SearchService.SearchCategory("data", "ExpData", false) { + @Override + public Class getRequiredPermission() + { + return DataClassReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { @@ -139,6 +145,12 @@ public Set getPermittedContainerIds(User user, Map co } }; public static final SearchService.SearchCategory expMediaDataCategory = new SearchService.SearchCategory("mediaData", "ExpData for media objects", false) { + @Override + public Class getRequiredPermission() + { + return MediaReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java index 152dd004add..c36c9e4b1bc 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; @@ -87,6 +88,12 @@ 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 Class getRequiredPermission() + { + return MediaReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { diff --git a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java index 1638e121f84..83c4a6b2a18 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; @@ -96,6 +97,12 @@ public class ExpSampleTypeImpl extends ExpIdentifiableEntityImpl private static final String mediaCategoryName = "mediaMaterialSource"; 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 Class getRequiredPermission() + { + return MediaReadPermission.class; + } + @Override public Set getPermittedContainerIds(User user, Map containers) { 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 c75670eec36..56c28c380aa 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..b47d12d4997 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,34 @@ 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.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; @@ -73,13 +84,88 @@ class SecurityQuery extends Query _recursive = searchScope.isRecursive(); _iTimer = iTimer; - _containerIds = searchScope.getSearchableContainers(user, currentContainer); - + // For now, perform the permission checking twice, old way and new way. This allows us to verify the results + // are identical and evaluate the performance benefit. TODO: Remove the block below and comparison asserts before merging. + iTimer.setPhase(SearchService.SEARCH_PHASE.buildSecurityFilterOld); + HashMap oldContainerIds = searchScope.getSearchableContainers(user, currentContainer); + HashMap> categoryContainers = new HashMap<>(); SearchService.get().getSearchCategories().forEach( - category -> { - _categoryContainers.put(category.getName(), category.getPermittedContainerIds(user, _containerIds)); - } + category -> categoryContainers.put(category.getName(), category.getPermittedContainerIds(user, oldContainerIds)) ); + iTimer.setPhase(SearchService.SEARCH_PHASE.buildSecurityFilter); + + _containerIds = searchScope.getSearchableContainers(user, currentContainer); + + assert oldContainerIds.equals(_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. + Set baseReadCategoryNames = new HashSet<>(); + MultiValuedMap, SearchCategory> categoriesByPermission = + groupCategoriesByRequiredPermission(SearchService.get().getSearchCategories(), baseReadCategoryNames); + + for (String categoryName : 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. + Set> requiredPermissions = categoriesByPermission.keySet(); + HashMap>> permissionsByPolicy = new HashMap<>(); + + if (!requiredPermissions.isEmpty()) + { + for (Container c : _containerIds.values()) + { + permissionsByPolicy.computeIfAbsent(c.getPolicy().getResourceId(), id -> { + Set> permitted = new HashSet<>(requiredPermissions); + permitted.retainAll(SecurityManager.getPermissions(c, user, null)); + return permitted; + }); + } + } + + categoriesByPermission.asMap().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); + }); + + assert categoryContainers.equals(_categoryContainers); + } + + /** + * 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 MultiValuedMap, SearchCategory> groupCategoriesByRequiredPermission( + Collection categories, Set baseReadCategoryNames) + { + MultiValuedMap, SearchCategory> categoriesByPermission = new ArrayListValuedHashMap<>(); + + for (SearchCategory category : categories) + { + Class requiredPermission = category.getRequiredPermission(); + + if (null == requiredPermission) + baseReadCategoryNames.add(category.getName()); + else + categoriesByPermission.put(requiredPermission, category); + } + + return categoriesByPermission; } @Override @@ -314,4 +400,78 @@ 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"); + Set baseReadCategoryNames = new HashSet<>(); + + MultiValuedMap, SearchCategory> categoriesByPermission = + SecurityQuery.groupCategoriesByRequiredPermission(List.of(wiki), baseReadCategoryNames); + + assertEquals(Set.of("wiki"), baseReadCategoryNames); + assertTrue(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); + Set baseReadCategoryNames = new HashSet<>(); + + MultiValuedMap, SearchCategory> categoriesByPermission = + SecurityQuery.groupCategoriesByRequiredPermission(List.of(assay, assayBatch, assayRun), baseReadCategoryNames); + + assertTrue(baseReadCategoryNames.isEmpty()); + assertEquals(Set.of(InsertPermission.class), categoriesByPermission.keySet()); + assertEquals(Set.of(assay, assayBatch, assayRun), Set.copyOf(categoriesByPermission.get(InsertPermission.class))); + } + + @Test + public void testCategoriesWithDifferentPermissionsAreNotGroupedTogether() + { + SearchCategory data = categoryRequiring("data", InsertPermission.class); + SearchCategory media = categoryRequiring("media", DeletePermission.class); + Set baseReadCategoryNames = new HashSet<>(); + + MultiValuedMap, SearchCategory> categoriesByPermission = + SecurityQuery.groupCategoriesByRequiredPermission(List.of(data, media), baseReadCategoryNames); + + assertEquals(Set.of(InsertPermission.class, DeletePermission.class), categoriesByPermission.keySet()); + assertEquals(List.of(data), categoriesByPermission.get(InsertPermission.class)); + assertEquals(List.of(media), categoriesByPermission.get(DeletePermission.class)); + } + + @Test + public void testMixOfBaseReadAndPermissionRequiringCategories() + { + SearchCategory wiki = new SearchCategory("wiki", "Wiki Pages"); + SearchCategory data = categoryRequiring("data", InsertPermission.class); + Set baseReadCategoryNames = new HashSet<>(); + + MultiValuedMap, SearchCategory> categoriesByPermission = + SecurityQuery.groupCategoriesByRequiredPermission(List.of(wiki, data), baseReadCategoryNames); + + assertEquals(Set.of("wiki"), baseReadCategoryNames); + assertEquals(List.of(data), categoriesByPermission.get(InsertPermission.class)); + } + } }