Skip to content

Commit 49800b1

Browse files
Improve scoping checks for objects (#7739)
#### Rationale We can improve our parameter validation #### Changes - New helpful base class for integration tests, `AbstractContainerScopingTest` - New test coverage - Assorted scoping checks
1 parent 32c28f5 commit 49800b1

30 files changed

Lines changed: 1091 additions & 83 deletions

File tree

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
/*
2+
* Copyright (c) 2026 LabKey Corporation
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.labkey.api.security.permissions;
17+
18+
import org.junit.After;
19+
import org.junit.Assert;
20+
import org.labkey.api.data.Container;
21+
import org.labkey.api.data.ContainerManager;
22+
import org.labkey.api.security.MutableSecurityPolicy;
23+
import org.labkey.api.security.SecurityManager;
24+
import org.labkey.api.security.SecurityPolicyManager;
25+
import org.labkey.api.security.User;
26+
import org.labkey.api.security.UserManager;
27+
import org.labkey.api.security.ValidEmail;
28+
import org.labkey.api.security.roles.Role;
29+
import org.labkey.api.util.JunitUtil;
30+
import org.labkey.api.util.TestContext;
31+
import org.labkey.api.view.ActionURL;
32+
import org.labkey.api.view.ViewServlet;
33+
import org.springframework.mock.web.MockHttpServletResponse;
34+
35+
import java.util.ArrayList;
36+
import java.util.List;
37+
import java.util.Map;
38+
39+
/**
40+
* Base class for "container scoping" (a.k.a. broken-object-level-authorization / BOLA / IDOR) integration tests. These
41+
* tests verify that an action whose {@code @RequiresPermission} annotation is correct for the current container still
42+
* rejects an object resolved by a global id that belongs to a <em>different</em> container.
43+
*
44+
* <p>The repeated scaffolding lives here so each subclass keeps only its data fixture and the action under test:
45+
* <ul>
46+
* <li>{@link #createContainer(String)} — make a throwaway child of the junit container (auto-cleaned).</li>
47+
* <li>{@link #createUserInRole(Container, Class)} — make a user with a role assigned in <em>one</em> folder only
48+
* (auto-cleaned). Use this to obtain a caller who is, say, admin in folder A but has no rights in folder B.</li>
49+
* <li>{@link #get(ActionURL, User)} / {@link #post(ActionURL, User)} — dispatch an in-JVM request as a given user
50+
* and inspect the {@link MockHttpServletResponse} status. Parameters travel on the {@link ActionURL}.</li>
51+
* </ul>
52+
*
53+
* <p>Note: WebDAV verbs (MOVE, PROPPATCH, ...) are not served through {@link ViewServlet} dispatch, so a WebDAV test
54+
* should still use this class for its container/user fixtures but drive the verb through {@code WebdavServlet} itself.
55+
*/
56+
public abstract class AbstractContainerScopingTest extends Assert
57+
{
58+
private static final Map<String, Object> FORM_HEADERS = Map.of("Content-Type", "application/x-www-form-urlencoded");
59+
60+
private final List<Container> _containers = new ArrayList<>();
61+
private final List<User> _users = new ArrayList<>();
62+
63+
/** The site-admin user (from {@link TestContext}) that owns the test fixtures. */
64+
protected User getAdmin()
65+
{
66+
return TestContext.get().getUser();
67+
}
68+
69+
/**
70+
* Create a throwaway child container of the junit container, named uniquely per test class, registered for
71+
* automatic cleanup. Callers pass a short local name (e.g. "A"/"B"/"Source"); the class name is prepended so two
72+
* test classes can both ask for "A" without colliding.
73+
*/
74+
protected Container createContainer(String name)
75+
{
76+
Container junit = JunitUtil.getTestContainer();
77+
Container c = ContainerManager.ensureContainer(junit.getParsedPath().append(getClass().getSimpleName() + "-" + name, true), getAdmin());
78+
_containers.add(c);
79+
return c;
80+
}
81+
82+
/**
83+
* Create a user that has {@code role} assigned in {@code scope} <em>only</em> (it has no rights in any other
84+
* container), registered for automatic cleanup. This is the canonical way to build a caller who is privileged in
85+
* one folder but not another. Do not use {@code LimitedUser} for this — that grants roles unconditionally in every
86+
* container.
87+
*/
88+
protected User createUserInRole(Container scope, Class<? extends Role> role) throws Exception
89+
{
90+
String email = getClass().getSimpleName().toLowerCase() + "-" + _users.size() + "@containerscoping.test";
91+
User user = SecurityManager.addUser(new ValidEmail(email), null).getUser();
92+
_users.add(user);
93+
grantRole(user, scope, role);
94+
return user;
95+
}
96+
97+
/**
98+
* Grant {@code role} to an existing {@code user} in {@code scope}, on top of any roles it already holds in that
99+
* container. Use this to build a caller with different roles in different folders (e.g. delete rights in a source
100+
* folder but only read access in a target folder).
101+
*/
102+
protected void grantRole(User user, Container scope, Class<? extends Role> role) throws Exception
103+
{
104+
MutableSecurityPolicy policy = new MutableSecurityPolicy(scope.getPolicy());
105+
policy.addRoleAssignment(user, role);
106+
SecurityPolicyManager.savePolicyForTests(policy, getAdmin());
107+
}
108+
109+
/**
110+
* Dispatch a GET to the action addressed by {@code url} as {@code user}. Put request parameters on the URL. No
111+
* request-body Content-Type is sent: a GET carries no body, and an "application/json" Content-Type would make an
112+
* API action ({@code ReadOnlyApiAction}) try to parse the empty body as JSON and fail with 400 before its
113+
* {@code execute()} runs. With no Content-Type the form binds from the URL parameters, as a real GET would.
114+
*/
115+
protected MockHttpServletResponse get(ActionURL url, User user) throws Exception
116+
{
117+
return ViewServlet.GET(url, user, Map.of());
118+
}
119+
120+
/** Dispatch a POST to the action addressed by {@code url} as {@code user}. Put request parameters on the URL. */
121+
protected MockHttpServletResponse post(ActionURL url, User user) throws Exception
122+
{
123+
return ViewServlet.POST(url, user, FORM_HEADERS, null);
124+
}
125+
126+
/** Assert that a dispatched response has the expected HTTP status code. */
127+
protected void assertStatus(int expected, MockHttpServletResponse response)
128+
{
129+
assertEquals("Unexpected HTTP status", expected, response.getStatus());
130+
}
131+
132+
@After
133+
public void cleanupContainerScopingFixtures()
134+
{
135+
User admin = getAdmin();
136+
137+
for (User user : _users)
138+
{
139+
try
140+
{
141+
UserManager.deleteUser(user.getUserId());
142+
}
143+
catch (Exception ignored)
144+
{
145+
}
146+
}
147+
_users.clear();
148+
149+
for (Container c : _containers)
150+
{
151+
try
152+
{
153+
ContainerManager.deleteAll(c, admin);
154+
}
155+
catch (Exception ignored)
156+
{
157+
}
158+
}
159+
_containers.clear();
160+
}
161+
}

core/src/org/labkey/core/CoreModule.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1394,11 +1394,13 @@ public Set<Class> getIntegrationTests()
13941394
AdminController.SerializationTest.class,
13951395
AdminController.TestCase.class,
13961396
AdminController.WorkbookDeleteTestCase.class,
1397+
AdminController.ImportFolderSourceScopingTestCase.class,
13971398
AllowListType.TestCase.class,
13981399
AttachmentServiceImpl.TestCase.class,
13991400
CoreController.TestCase.class,
14001401
DataRegion.TestCase.class,
14011402
DavController.TestCase.class,
1403+
DavController.MoveActionContainerScopingTestCase.class,
14021404
EmailServiceImpl.TestCase.class,
14031405
FilesSiteSettingsAction.TestCase.class,
14041406
LoggerController.TestCase.class,

core/src/org/labkey/core/admin/AdminController.java

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,7 @@
209209
import org.labkey.api.security.impersonation.RoleImpersonationContextFactory;
210210
import org.labkey.api.security.impersonation.UserImpersonationContextFactory;
211211
import org.labkey.api.security.permissions.AbstractActionPermissionTest;
212+
import org.labkey.api.security.permissions.AbstractContainerScopingTest;
212213
import org.labkey.api.security.permissions.AdminOperationsPermission;
213214
import org.labkey.api.security.permissions.AdminPermission;
214215
import org.labkey.api.security.permissions.ApplicationAdminPermission;
@@ -5393,6 +5394,10 @@ public boolean handlePost(ImportFolderForm form, BindException errors) throws Ex
53935394
if (!StringUtils.isEmpty(form.getSourceTemplateFolder()))
53945395
{
53955396
fiConfig = getFolderImportConfigFromTemplateFolder(form, pipelineUnzipDir, errors);
5397+
if (fiConfig == null || errors.hasErrors())
5398+
{
5399+
return false;
5400+
}
53965401
}
53975402
else
53985403
{
@@ -5488,10 +5493,16 @@ public boolean handlePost(ImportFolderForm form, BindException errors) throws Ex
54885493

54895494
private FolderImportConfig getFolderImportConfigFromTemplateFolder(final ImportFolderForm form, final Path pipelineUnzipDir, final BindException errors) throws Exception
54905495
{
5491-
// user choose to import from a template source folder
5496+
// user chose to import from a template source folder
54925497
Container sourceContainer = form.getSourceTemplateFolderContainer();
54935498

5494-
// In order to support the Advanced import options to import into multiple target folders we need to zip
5499+
if (sourceContainer == null || !sourceContainer.hasPermission(getUser(), AdminPermission.class))
5500+
{
5501+
errors.reject(SpringActionController.ERROR_MSG, "You do not have permission to import from the specified source folder.");
5502+
return null;
5503+
}
5504+
5505+
// To support the Advanced import options to import into multiple target folders we need to zip
54955506
// the source template folder so that the zip file can be passed to the pipeline processes.
54965507
FolderExportContext ctx = new FolderExportContext(getUser(), sourceContainer,
54975508
getRegisteredFolderWritersForImplicitExport(sourceContainer), "new", false,
@@ -12286,4 +12297,28 @@ protected static void doCleanup() throws Exception
1228612297
}
1228712298
}
1228812299
}
12300+
12301+
public static class ImportFolderSourceScopingTestCase extends AbstractContainerScopingTest
12302+
{
12303+
@Test
12304+
public void testImportFromTemplateRequiresSourceAdmin() throws Exception
12305+
{
12306+
Container dest = createContainer("Dest");
12307+
Container source = createContainer("Source");
12308+
User destAdminOnly = createUserInRole(dest, FolderAdminRole.class);
12309+
12310+
ActionURL url = new ActionURL(ImportFolderAction.class, dest)
12311+
.addParameter("sourceTemplateFolder", source.getPath())
12312+
.addParameter("sourceTemplateFolderId", source.getId());
12313+
MockHttpServletResponse resp = post(url, destAdminOnly);
12314+
12315+
// The fix rejects the import and reshows the form (200) rather than redirecting to success (302), with a
12316+
// source-permission error message in the rendered content.
12317+
assertStatus(HttpServletResponse.SC_OK, resp);
12318+
assertTrue("Expected a source-permission rejection message, content was: " + resp.getContentAsString(),
12319+
resp.getContentAsString().contains("permission to import from the specified source folder"));
12320+
12321+
// Positive control performed in S3ImportTest.testS3Import(). Difficult to mock here due to pipeline job
12322+
}
12323+
}
1228912324
}

0 commit comments

Comments
 (0)