Skip to content

Add search permission filtering coverage for SearchScope and Security… #8137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
labkey-adam merged 1 commit into from
Oct 5, 2026
Merged

Add search permission filtering coverage for SearchScope and Security… #8137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
234 changes: 203 additions & 31 deletions api/src/org/labkey/api/search/SearchScope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,41 @@
*/
package org.labkey.api.search;

import org.junit.After;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import org.labkey.api.data.Container;
import org.labkey.api.data.Container.LockState;
import org.labkey.api.data.ContainerManager;
import org.labkey.api.data.ContainerType;
import org.labkey.api.data.WorkbookContainerType;
import org.labkey.api.security.ClonedUser;
import org.labkey.api.security.Group;
import org.labkey.api.security.MutableSecurityPolicy;
import org.labkey.api.security.PermissionsContext;
import org.labkey.api.security.SecurityManager;
import org.labkey.api.security.SecurityPolicyManager;
import org.labkey.api.security.User;
import org.labkey.api.security.UserManager;
import org.labkey.api.security.ValidEmail;
import org.labkey.api.security.impersonation.RoleImpersonationContextFactory;
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.security.roles.EditorRole;
import org.labkey.api.security.roles.ReaderRole;
import org.labkey.api.security.roles.RoleManager;
import org.labkey.api.test.TestTimeout;
import org.labkey.api.util.SafeToRenderEnum;
import org.labkey.api.util.TestContext;

import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -233,55 +243,96 @@ private static String getPolicyKey(Container c)
return c.getPolicy().getResourceId() + "|" + (null == project ? "" : project.getId());
}

// Container deletion dominates; each test container takes several seconds to delete
@TestTimeout(240)
public static class TestCase extends Assert
{
private static final String PROJECT_NAME = "SearchScopeTestProject";
private static final String OTHER_PROJECT_NAME = "SearchScopeTestOtherProject";
private static final String ROOT_POLICY_PROJECT_NAME = "SearchScopeTestRootPolicyProject";
private static final String EMAIL = "search_scope_test@test.com";

private User _user;
private Container _project;
private Container _inherited;
private Container _workbook;
private Container _editable;
private Container _restricted;

@Before
public void setUp() throws Exception
private static final String GROUP_MEMBER_EMAIL = "search_scope_group_test@test.com";
private static final Set<Class<? extends Permission>> EXTRA_PERMISSIONS = Set.of(InsertPermission.class, DeletePermission.class);

private static User _admin;
private static User _user;
private static User _groupMember;
private static Container _project;
private static Container _inherited;
private static Container _workbook;
private static Container _editable;
private static Container _restricted;
private static Container _unsearchable;
private static Container _otherProject;
private static Container _rootPolicyProject;

@BeforeClass
public static void setUp() throws Exception
{
cleanup();
User admin = TestContext.get().getUser();
_admin = TestContext.get().getUser();
_user = SecurityManager.addUser(new ValidEmail(EMAIL), null).getUser();
_groupMember = SecurityManager.addUser(new ValidEmail(GROUP_MEMBER_EMAIL), null).getUser();

_project = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_NAME, admin);
_project = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_NAME, _admin);
Group readers = SecurityManager.createGroup(_project, "Readers", _admin);
SecurityManager.addMember(readers, _groupMember);
MutableSecurityPolicy projectPolicy = new MutableSecurityPolicy(_project.getPolicy());
projectPolicy.addRoleAssignment(_user, ReaderRole.class);
SecurityPolicyManager.savePolicyForTests(projectPolicy, admin);
projectPolicy.addRoleAssignment(readers, ReaderRole.class);
SecurityPolicyManager.savePolicyForTests(projectPolicy, _admin);

// Subfolders created by an admin get an admin-only policy, so explicitly inherit
_inherited = ContainerManager.createContainer(_project, "Inherited", admin);
SecurityManager.setInheritPermissions(_inherited);
_workbook = ContainerManager.createContainer(_inherited, null, "Workbook", null, WorkbookContainerType.NAME, admin);
_inherited = createInheritingFolder(_project, "Inherited");
_workbook = ContainerManager.createContainer(_inherited, null, "Workbook", null, WorkbookContainerType.NAME, _admin);
assertEquals(_project.getPolicy().getResourceId(), _workbook.getPolicy().getResourceId());

_editable = ContainerManager.createContainer(_project, "Editable", admin);
_editable = ContainerManager.createContainer(_project, "Editable", _admin);
MutableSecurityPolicy editablePolicy = new MutableSecurityPolicy(_editable);
editablePolicy.addRoleAssignment(_user, EditorRole.class);
SecurityPolicyManager.savePolicyForTests(editablePolicy, admin);
SecurityPolicyManager.savePolicyForTests(editablePolicy, _admin);

_restricted = ContainerManager.createContainer(_project, "Restricted", _admin);
SecurityPolicyManager.savePolicyForTests(new MutableSecurityPolicy(_restricted), _admin);

_unsearchable = createInheritingFolder(_project, "Unsearchable");
ContainerManager.updateSearchable(_unsearchable, false, _admin);
_unsearchable = ContainerManager.getForId(_unsearchable.getId());

_restricted = ContainerManager.createContainer(_project, "Restricted", admin);
SecurityPolicyManager.savePolicyForTests(new MutableSecurityPolicy(_restricted), admin);
_otherProject = ContainerManager.createContainer(ContainerManager.getRoot(), OTHER_PROJECT_NAME, _admin);
MutableSecurityPolicy otherPolicy = new MutableSecurityPolicy(_otherProject.getPolicy());
otherPolicy.addRoleAssignment(_user, ReaderRole.class);
SecurityPolicyManager.savePolicyForTests(otherPolicy, _admin);

// A project with no policy of its own resolves to root's policy, sharing its resource ID with root and every other such project
_rootPolicyProject = ContainerManager.createContainer(ContainerManager.getRoot(), ROOT_POLICY_PROJECT_NAME, _admin);
SecurityPolicyManager.deletePolicy(_rootPolicyProject);
assertEquals(ContainerManager.getRoot().getPolicy().getResourceId(), _rootPolicyProject.getPolicy().getResourceId());
}

private static Container createInheritingFolder(Container parent, String name)
{
Container c = ContainerManager.createContainer(parent, name, _admin);
SecurityManager.setInheritPermissions(c);
return c;
}

@After
public void cleanup() throws Exception
@AfterClass
public static void cleanup() throws Exception
{
Container project = ContainerManager.getForPath(PROJECT_NAME);
if (null != project)
ContainerManager.deleteAll(project, TestContext.get().getUser());
for (String name : List.of(PROJECT_NAME, OTHER_PROJECT_NAME, ROOT_POLICY_PROJECT_NAME))
{
Container project = ContainerManager.getForPath(name);
if (null != project)
ContainerManager.deleteAll(project, TestContext.get().getUser());
}

User user = UserManager.getUser(new ValidEmail(EMAIL));
if (null != user)
UserManager.deleteUser(user.getUserId());
for (String email : List.of(EMAIL, GROUP_MEMBER_EMAIL))
{
User user = UserManager.getUser(new ValidEmail(email));
if (null != user)
UserManager.deleteUser(user.getUserId());
}
}

@Test
Expand All @@ -298,7 +349,7 @@ public void testResolvedPermissionsMatchPerContainerChecks()
assertEquals(c.getPath(), canRead && c.hasPermission(_user, InsertPermission.class), insertable.contains(c.getId()));
}

assertEquals(Set.of(_project.getId(), _inherited.getId(), _workbook.getId(), _editable.getId()), result.readable().keySet());
assertEquals(Set.of(_project.getId(), _inherited.getId(), _workbook.getId(), _editable.getId(), _unsearchable.getId()), result.readable().keySet());
assertEquals(Set.of(_editable.getId()), insertable);
}

Expand All @@ -312,6 +363,127 @@ public void testScopes()
Set<String> recursive = FolderAndSubfolders.getSearchableContainers(_user, _project, Set.of()).readable().keySet();
assertTrue(recursive.containsAll(Set.of(_project.getId(), _inherited.getId(), _editable.getId())));
assertFalse(recursive.contains(_restricted.getId()));
assertFalse(recursive.contains(_unsearchable.getId()));

// Searching from within an unsearchable folder still includes it
assertTrue(FolderAndSubfolders.getSearchableContainers(_user, _unsearchable, Set.of()).readable().containsKey(_unsearchable.getId()));
}

@Test
public void testEveryScopeMatchesPerContainerChecks()
{
assertMatchesPerContainerChecks(getUsers(), List.of(_project, _inherited, _workbook, _editable, _restricted, _unsearchable, _otherProject, _rootPolicyProject));
}

@Test
public void testImpersonationRestrictedToProject()
{
User impersonator = getProjectImpersonator();
assertEquals(Set.of(), Folder.getSearchableContainers(impersonator, _otherProject, Set.of()).readable().keySet());

Set<String> all = All.getSearchableContainers(impersonator, _project, Set.of()).readable().keySet();
assertTrue(all.contains(_project.getId()));
assertFalse(all.contains(_otherProject.getId()));
assertFalse(all.contains(_rootPolicyProject.getId()));
}

@Test
public void testLockedProject()
{
ContainerManager.updateLockState(_otherProject, LockState.Inaccessible, () -> {});

try
{
assertMatchesPerContainerChecks(getUsers(), List.of(_project, ContainerManager.getForId(_otherProject.getId())));
}
finally
{
ContainerManager.updateLockState(_otherProject, LockState.Unlocked, () -> {});
}
}

private Map<String, User> getUsers()
{
Map<String, User> users = new LinkedHashMap<>();
users.put("reader", _user);
users.put("group member", _groupMember);
users.put("site admin", _admin);
users.put("guest", User.guest);
users.put("project impersonator", getProjectImpersonator());
return users;
}

// Admin impersonating Reader, restricted to _project
private User getProjectImpersonator()
{
RoleImpersonationContextFactory factory = new RoleImpersonationContextFactory(_project, _admin, Set.of(RoleManager.getRole(ReaderRole.class)), Set.of(), null);
return new ImpersonatingUser(_admin, factory.getImpersonationContext());
}

private static class ImpersonatingUser extends ClonedUser
{
ImpersonatingUser(User user, PermissionsContext ctx)
{
super(user, ctx);
}
}

private void assertMatchesPerContainerChecks(Map<String, User> users, List<Container> currentContainers)
{
for (SearchScope scope : SearchScope.values())
{
for (Map.Entry<String, User> entry : users.entrySet())
{
for (Container current : currentContainers)
{
// All is rooted at the root regardless of current container; one pass is enough
if (scope == All && !current.equals(currentContainers.getFirst()))
continue;

User user = entry.getValue();
String msg = scope + " as " + entry.getKey() + " from " + current.getPath();
Set<Container> expected = getExpectedReadable(scope, user, current);
SearchableContainers actual = scope.getSearchableContainers(user, current, EXTRA_PERMISSIONS);

assertEquals(msg, getIds(expected.stream()), actual.readable().keySet());

for (Class<? extends Permission> permission : EXTRA_PERMISSIONS)
assertEquals(msg + ", " + permission.getSimpleName(), getIds(expected.stream().filter(c -> c.hasPermission(user, permission))), actual.containerIdsByPermission().get(permission));
}
}
}
}

// The pre-optimization algorithm, which checks permissions on every container individually
private static Set<Container> getExpectedReadable(SearchScope scope, User user, Container current)
{
Container root = scope.getRoot(current);
Set<Container> expected = new HashSet<>();

if (scope.isRecursive())
{
ContainerManager.getAllChildren(root, user).stream()
.filter(c -> (c.isSearchable() || c.equals(current)) && (c.isContainerFor(ContainerType.DataType.search) || c.shouldDisplay(user)))
.forEach(expected::add);
}
else
{
List<Container> roots = scope == FolderAndProject || scope == FolderAndProjectAndShared ? List.of(root, root.getProject()) : List.of(root);
roots.stream()
.filter(c -> c.hasPermission(user, ReadPermission.class))
.forEach(expected::add);
}

Container shared = ContainerManager.getSharedContainer();
if (scope.includeShared() && shared.hasPermission(user, ReadPermission.class))
expected.add(shared);

return expected;
}

private static Set<String> getIds(Stream<Container> containers)
{
return containers.map(Container::getId).collect(Collectors.toSet());
}
}
}
3 changes: 2 additions & 1 deletion search/src/org/labkey/search/SearchModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,8 @@ private void reindexIfNeeded(@NotNull SearchService ss)
(
LuceneSearchServiceImpl.TestCase.class,
LuceneSearchServiceImpl.TikaTestCase.class,
LuceneSearchServiceImpl.IndexWriterTestCase.class
LuceneSearchServiceImpl.IndexWriterTestCase.class,
SecurityQuery.FilterTestCase.class
);
}

Expand Down
Loading
Loading