Skip to content

Commit f601fe8

Browse files
vagishaclaude
vagisha
and
claude
committed
Added a self-service group-change security test and a distinct rejection status
* ChangeGroupsApiAction returns TARGET_NOT_ALLOWED when validateGroupChangeTarget refuses a privileged, site, or cross-project target. NO_PERMISSIONS still covers callers that are ineligible for the transition. * Added SignUpGroupChangeSecurityTest. As a non-admin it plants each dangerous transition rule, then verifies the move is refused and membership unchanged, while a plain same-project move still succeeds. Co-Authored-By: Claude <[email protected]>
1 parent ee4451e commit f601fe8

2 files changed

Lines changed: 318 additions & 1 deletion

File tree

‎signup/src/org/labkey/signup/SignUpController.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -794,6 +794,7 @@ private static String validateGroupChangeTarget(Group oldgroup, Group newgroup)
794794
if (!c.isContainerFor(ContainerType.DataType.permissions))
795795
continue;
796796
if (c.hasPermission(newgroup, AdminPermission.class))
797+
797798
return "target group carries administrative permission";
798799
}
799800
return null;
@@ -927,7 +928,11 @@ public ApiResponse execute(AddGroupChangeForm addGroupChangeForm, BindException
927928
{
928929
_log.warn("Rejected self-service group change for user {} (group {} -> {}): {}",
929930
user.getEmail(), addGroupChangeForm.getOldgroup(), addGroupChangeForm.getNewgroup(), denyReason);
930-
response.put("status", "NO_PERMISSIONS");
931+
// A configured rule points at a target the self-service flow must never grant (privileged,
932+
// site, or other-project). Report this with its own status so a refused misconfiguration is
933+
// distinguishable from an ineligible caller (NO_PERMISSIONS above). The specific reason is
934+
// logged server-side only, not returned to the caller.
935+
response.put("status", "TARGET_NOT_ALLOWED");
931936
return response;
932937
}
933938
try (DbScope.Transaction transaction = CoreSchema.getInstance().getSchema().getScope().ensureTransaction())

‎signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java‎

Lines changed: 312 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,312 @@
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.test.tests.signup;
17+
18+
import org.apache.hc.client5.http.classic.methods.HttpPost;
19+
import org.apache.hc.client5.http.entity.UrlEncodedFormEntity;
20+
import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
21+
import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
22+
import org.apache.hc.core5.http.HttpStatus;
23+
import org.apache.hc.core5.http.NameValuePair;
24+
import org.apache.hc.core5.http.io.entity.EntityUtils;
25+
import org.apache.hc.core5.http.message.BasicNameValuePair;
26+
import org.apache.hc.core5.http.protocol.HttpContext;
27+
import org.junit.After;
28+
import org.junit.BeforeClass;
29+
import org.junit.Test;
30+
import org.junit.experimental.categories.Category;
31+
import org.labkey.remoteapi.CommandResponse;
32+
import org.labkey.remoteapi.Connection;
33+
import org.labkey.remoteapi.SimplePostCommand;
34+
import org.labkey.test.BaseWebDriverTest;
35+
import org.labkey.test.WebTestHelper;
36+
import org.labkey.test.categories.External;
37+
import org.labkey.test.categories.MacCossLabModules;
38+
import org.labkey.test.util.APITestHelper;
39+
import org.labkey.test.util.ApiPermissionsHelper;
40+
import org.labkey.test.util.LogMethod;
41+
import org.labkey.test.util.PermissionsHelper.MemberType;
42+
import org.labkey.test.util.PermissionsHelper.PrincipalType;
43+
44+
import java.io.IOException;
45+
import java.util.ArrayList;
46+
import java.util.List;
47+
import java.util.Map;
48+
49+
import static org.junit.Assert.assertEquals;
50+
import static org.junit.Assert.assertFalse;
51+
import static org.junit.Assert.assertTrue;
52+
import static org.labkey.test.util.PermissionsHelper.FOLDER_ADMIN_ROLE;
53+
54+
/**
55+
* The self-service group change (ChangeGroupsApiAction) must not let a logged-in user escalate their own privileges.
56+
*
57+
* The transition rule map (the site-global "group A may move to group B" list) is admin-configured
58+
* and could be pointed at a privileged target by mistake. ChangeGroupsApiAction therefore re-validates
59+
* the resolved groups with validateGroupChangeTarget before it changes any membership, and rejects the
60+
* move with status TARGET_NOT_ALLOWED when the target:
61+
* - is a site group rather than a project group,
62+
* - is in a different project than the source group, or
63+
* - carries administrative permission anywhere in its project or its subfolders.
64+
*
65+
* This test plants each of those dangerous rules on purpose, then confirms that a non-admin member of
66+
* the source group is refused every escalating move (and is neither added to the target nor removed from
67+
* the source), while a legitimate move to a plain non-admin project group in the same project still works.
68+
*
69+
* Each escalating case needs its transition rule planted so the attempt gets past the "rule exists" gate and
70+
* actually reaches validateGroupChangeTarget. A target with no rule is refused before that check, with
71+
* NO_PERMISSIONS. The test drives a no-rule move on purpose and asserts NO_PERMISSIONS, so that path stays
72+
* distinguishable from the TARGET_NOT_ALLOWED path.
73+
*
74+
* Design notes:
75+
* - The live client (a skyline.ms wiki page) is not in the module, so this drives the server actions
76+
* directly: the admin AddGroupChangeProperty action to plant rules, and the ChangeGroupsApi action as
77+
* the user to attempt the moves.
78+
* - The transition rule map is site-global (no container), so the planted rules are removed in @After
79+
* (not doCleanup) so they are cleaned up even on a local clean=false run.
80+
*/
81+
@Category({External.class, MacCossLabModules.class})
82+
@BaseWebDriverTest.ClassTimeout(minutes = 2)
83+
public class SignUpGroupChangeSecurityTest extends BaseWebDriverTest
84+
{
85+
private static final String PROJECT_1 = "SignUpSecurityTest P1";
86+
private static final String PROJECT_2 = "SignUpSecurityTest P2";
87+
private static final String SUBFOLDER = "Sub";
88+
89+
private static final String GROUP_SOURCE = "SignupSource";
90+
private static final String GROUP_TARGET_OK = "SignupTargetOk";
91+
private static final String GROUP_TARGET_ADMIN = "SignupTargetAdmin";
92+
private static final String GROUP_TARGET_SUBADMIN = "SignupTargetSubAdmin";
93+
private static final String GROUP_TARGET_CROSS = "SignupTargetCross";
94+
private static final String SITE_GROUP = "SignupSiteGroup";
95+
96+
// Group ids captured during setup, used both for the rule map and for the ChangeGroupsApi params.
97+
private static int idSource;
98+
private static int idTargetOk;
99+
private static int idTargetAdmin;
100+
private static int idTargetSubAdmin;
101+
private static int idTargetCross;
102+
private static int idSiteGroup;
103+
104+
private static final String USER = "[email protected]";
105+
106+
// Password for the user account, so we can POST to ChangeGroupsApi as that user.
107+
private static String userPassword;
108+
109+
// Transition rules this test planted, so @After can remove them from the site-global rule map.
110+
private final List<int[]> _plantedRules = new ArrayList<>();
111+
112+
@Override
113+
protected String getProjectName()
114+
{
115+
return PROJECT_1;
116+
}
117+
118+
@BeforeClass
119+
public static void setupProject()
120+
{
121+
SignUpGroupChangeSecurityTest init = getCurrentTest();
122+
init.doSetup();
123+
}
124+
125+
@LogMethod
126+
private void doSetup()
127+
{
128+
_containerHelper.createProject(PROJECT_1, null);
129+
_containerHelper.createSubfolder(PROJECT_1, SUBFOLDER);
130+
_containerHelper.createProject(PROJECT_2, null);
131+
132+
ApiPermissionsHelper perms = new ApiPermissionsHelper(this);
133+
idSource = perms.createProjectGroup(GROUP_SOURCE, PROJECT_1);
134+
idTargetOk = perms.createProjectGroup(GROUP_TARGET_OK, PROJECT_1);
135+
idTargetAdmin = perms.createProjectGroup(GROUP_TARGET_ADMIN, PROJECT_1);
136+
idTargetSubAdmin = perms.createProjectGroup(GROUP_TARGET_SUBADMIN, PROJECT_1);
137+
idTargetCross = perms.createProjectGroup(GROUP_TARGET_CROSS, PROJECT_2);
138+
idSiteGroup = perms.createGlobalPermissionsGroup(SITE_GROUP);
139+
140+
// TargetAdmin carries admin in the P1 project; TargetSubAdmin carries admin only in the P1/Sub
141+
// subfolder. Assigning a role in the subfolder gives it its own permission policy (breaks
142+
// inheritance), which is what the subfolder branch of validateGroupChangeTarget looks for.
143+
perms.addMemberToRole(idTargetAdmin, FOLDER_ADMIN_ROLE, "/" + PROJECT_1);
144+
perms.addMemberToRole(idTargetSubAdmin, FOLDER_ADMIN_ROLE, "/" + PROJECT_1 + "/" + SUBFOLDER);
145+
146+
// A non-admin user who is a member of the source group. All rejection cases depend on the user
147+
// being in Source, because the action checks source-group membership before it validates the target.
148+
_userHelper.createUser(USER);
149+
userPassword = setInitialPassword(USER);
150+
perms.addUserToProjGroup(USER, PROJECT_1, GROUP_SOURCE);
151+
}
152+
153+
@Test
154+
public void testSelfServiceGroupChangeRejectsPrivilegeEscalation() throws Exception
155+
{
156+
// Plant the four dangerous rules so each escalating attempt gets past the "rule exists" check and
157+
// actually reaches validateGroupChangeTarget. TargetOk's rule is planted later, just before the
158+
// happy path, so it can first stand in for the "no rule configured" case below.
159+
addTransitionRule(idSource, idTargetAdmin);
160+
addTransitionRule(idSource, idTargetSubAdmin);
161+
addTransitionRule(idSource, idTargetCross);
162+
addTransitionRule(idSource, idSiteGroup);
163+
164+
Connection userConnection = new Connection(WebTestHelper.getBaseURL(), USER, userPassword);
165+
166+
// Contrast case: a valid same-project non-admin target with no rule configured is rejected before
167+
// validateGroupChangeTarget, with NO_PERMISSIONS. Asserting this keeps the two rejection reasons distinguishable.
168+
assertMoveNotEligible(userConnection, idTargetOk, GROUP_TARGET_OK,
169+
"no transition rule is configured for the target");
170+
171+
// Group transition validation rejections. Run these while the user is still in Source; each must
172+
// leave membership untouched and report TARGET_NOT_ALLOWED.
173+
assertMoveRejected(userConnection, idTargetAdmin, GROUP_TARGET_ADMIN, PROJECT_1,
174+
"target group carries admin permission in its project");
175+
assertMoveRejected(userConnection, idTargetSubAdmin, GROUP_TARGET_SUBADMIN, PROJECT_1,
176+
"target group carries admin permission in a subfolder");
177+
assertMoveRejected(userConnection, idTargetCross, GROUP_TARGET_CROSS, PROJECT_2,
178+
"target group is in a different project");
179+
assertMoveRejected(userConnection, idSiteGroup, SITE_GROUP, "/",
180+
"target is a site group, not a project group");
181+
182+
// Legitimate move, run last because it actually moves the user out of the source group.
183+
addTransitionRule(idSource, idTargetOk);
184+
assertMoveSucceeded(userConnection, idTargetOk, GROUP_TARGET_OK);
185+
}
186+
187+
private void assertMoveRejected(Connection userConnection, int targetId, String targetGroup,
188+
String targetContainer, String because) throws Exception
189+
{
190+
String status = postGroupChange(userConnection, idSource, targetId);
191+
assertEquals("Escalating move should be refused with TARGET_NOT_ALLOWED because " + because,
192+
"TARGET_NOT_ALLOWED", status);
193+
194+
ApiPermissionsHelper perms = new ApiPermissionsHelper(this);
195+
assertFalse("User must not have been added to " + targetGroup + " (" + because + ")",
196+
perms.isUserInGroup(USER, targetGroup, targetContainer, PrincipalType.USER));
197+
assertTrue("User must remain in the source group after a refused move (" + because + ")",
198+
perms.isUserInGroup(USER, GROUP_SOURCE, PROJECT_1, PrincipalType.USER));
199+
}
200+
201+
// A move rejected before the group transition validation runs (e.g. no rule configured, or caller not in
202+
// the source group) reports NO_PERMISSIONS, distinct from the validation's TARGET_NOT_ALLOWED.
203+
private void assertMoveNotEligible(Connection userConnection, int targetId, String targetGroup,
204+
String because) throws Exception
205+
{
206+
String status = postGroupChange(userConnection, idSource, targetId);
207+
assertEquals("Move should be refused with NO_PERMISSIONS because " + because, "NO_PERMISSIONS", status);
208+
209+
ApiPermissionsHelper perms = new ApiPermissionsHelper(this);
210+
assertFalse("User must not have been added to " + targetGroup + " (" + because + ")",
211+
perms.isUserInGroup(USER, targetGroup, PROJECT_1, PrincipalType.USER));
212+
assertTrue("User must remain in the source group after a refused move (" + because + ")",
213+
perms.isUserInGroup(USER, GROUP_SOURCE, PROJECT_1, PrincipalType.USER));
214+
}
215+
216+
private void assertMoveSucceeded(Connection userConnection, int targetId, String targetGroup) throws Exception
217+
{
218+
String status = postGroupChange(userConnection, idSource, targetId);
219+
assertEquals("A legitimate move to a non-admin project group should succeed", "USER_MOVED_SUCCESS", status);
220+
221+
ApiPermissionsHelper perms = new ApiPermissionsHelper(this);
222+
assertTrue("User should now be a member of " + targetGroup,
223+
perms.isUserInGroup(USER, targetGroup, PROJECT_1, PrincipalType.USER));
224+
assertFalse("User should have been removed from the source group",
225+
perms.isUserInGroup(USER, GROUP_SOURCE, PROJECT_1, PrincipalType.USER));
226+
}
227+
228+
// Posts to ChangeGroupsApi as the user and returns the response status string.
229+
private String postGroupChange(Connection userConnection, int oldGroup, int newGroup) throws Exception
230+
{
231+
SimplePostCommand command = new SimplePostCommand("signup", "changeGroupsApi");
232+
command.setParameters(Map.of("oldgroup", oldGroup, "newgroup", newGroup));
233+
CommandResponse response = command.execute(userConnection, "/");
234+
return (String) response.getProperty("status");
235+
}
236+
237+
private void addTransitionRule(int oldGroup, int newGroup) throws IOException
238+
{
239+
postAdminGroupChangeForm("addGroupChangeProperty", oldGroup, newGroup);
240+
_plantedRules.add(new int[]{oldGroup, newGroup});
241+
}
242+
243+
// Drives the site-admin AddGroupChangeProperty / RemoveGroupChangeProperty form actions. These are
244+
// FormHandlerActions (they redirect rather than return JSON), so this posts form-encoded fields as the
245+
// logged-in admin: injectCookies carries the admin session and CSRF token, and getBasicHttpContext adds
246+
// preemptive basic auth.
247+
private void postAdminGroupChangeForm(String action, int oldGroup, int newGroup) throws IOException
248+
{
249+
HttpPost post = new HttpPost(WebTestHelper.buildURL("signup", "/", action));
250+
List<NameValuePair> form = List.of(
251+
new BasicNameValuePair("oldgroup", String.valueOf(oldGroup)),
252+
new BasicNameValuePair("newgroup", String.valueOf(newGroup)));
253+
post.setEntity(new UrlEncodedFormEntity(form));
254+
APITestHelper.injectCookies(post);
255+
256+
HttpContext context = WebTestHelper.getBasicHttpContext();
257+
try (CloseableHttpClient client = WebTestHelper.getHttpClient();
258+
CloseableHttpResponse response = client.execute(post, context))
259+
{
260+
int code = response.getCode();
261+
assertTrue(action + " returned unexpected HTTP status " + code,
262+
code == HttpStatus.SC_OK || code == HttpStatus.SC_MOVED_TEMPORARILY);
263+
EntityUtils.consumeQuietly(response.getEntity());
264+
}
265+
}
266+
267+
@After
268+
public void removePlantedRules()
269+
{
270+
// The transition rule map is site-global, so remove the rules this test planted even when project
271+
// cleanup is skipped (clean=false). Runs after the test method.
272+
if (_plantedRules.isEmpty())
273+
return;
274+
275+
ensureSignedInAsPrimaryTestUser();
276+
for (int[] rule : _plantedRules)
277+
{
278+
try
279+
{
280+
postAdminGroupChangeForm("removeGroupChangeProperty", rule[0], rule[1]);
281+
}
282+
catch (IOException e)
283+
{
284+
log("Failed to remove planted transition rule " + rule[0] + " -> " + rule[1] + ": " + e.getMessage());
285+
}
286+
}
287+
_plantedRules.clear();
288+
}
289+
290+
@Override
291+
protected void doCleanup(boolean afterTest)
292+
{
293+
// The site group is not scoped to a project, so it survives project deletion and must be removed
294+
// explicitly.
295+
new ApiPermissionsHelper(this).deleteGroup(SITE_GROUP, false);
296+
_userHelper.deleteUsers(false, USER);
297+
_containerHelper.deleteProject(PROJECT_1, afterTest);
298+
_containerHelper.deleteProject(PROJECT_2, afterTest);
299+
}
300+
301+
@Override
302+
public List<String> getAssociatedModules()
303+
{
304+
return List.of("signup");
305+
}
306+
307+
@Override
308+
protected BrowserType bestBrowser()
309+
{
310+
return BrowserType.CHROME;
311+
}
312+
}

0 commit comments

Comments
 (0)