From 0a21a772019db77fbc98cfb05393647dda7118f4 Mon Sep 17 00:00:00 2001 From: vagisha Date: Tue, 28 Jul 2026 09:29:41 -0700 Subject: [PATCH 1/2] Address security issues in the signup module (#667) - **Group-change validation.** The self-service group change re-validates the resolved groups before changing any membership. Both must be project groups in the same project, and the target must grant no more than read access, with no write or administrative permission anywhere in the project tree. Requests that don't meet these constraints are refused with the generic NO_PERMISSIONS status (the specific reason is logged server-side), and successful moves are audited. - **Consistent signup responses.** The signup endpoints return the same response whether or not an address already has an account, including when email delivery fails. The owner of an existing address receives a notice pointing to the password-reset flow, and the existing account is never modified. - **Auditing.** The administrative configuration actions now write an audit event on success. - **Error handling.** Mail-send errors are logged server-side, and callers receive a generic message. - **Test.** Added `SignUpGroupChangeSecurityTest`, covering the group-change validation and its auditing. Co-Authored-By: Claude --- .../org/labkey/signup/SignUpController.java | 202 +++++++--- .../signup/SignUpGroupChangeSecurityTest.java | 367 ++++++++++++++++++ 2 files changed, 511 insertions(+), 58 deletions(-) create mode 100644 signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 127febe0..a51533fd 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -32,6 +32,8 @@ import org.labkey.api.action.SimpleViewAction; import org.labkey.api.action.SpringActionController; import org.labkey.api.admin.AdminUrls; +import org.labkey.api.audit.AuditLogService; +import org.labkey.api.audit.ClientApiAuditProvider; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.CoreSchema; @@ -44,6 +46,7 @@ import org.labkey.api.security.DbLoginService; import org.labkey.api.security.Group; import org.labkey.api.security.LoginManager; +import org.labkey.api.security.LoginUrls; import org.labkey.api.security.RequiresLogin; import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; @@ -52,11 +55,16 @@ import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.security.ValidEmail; +import org.labkey.api.security.permissions.AdminPermission; +import org.labkey.api.security.permissions.DeletePermission; +import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.settings.LookAndFeelProperties; import org.labkey.api.util.ButtonBuilder; import org.labkey.api.util.ConfigurationException; import org.labkey.api.util.DOM; +import org.labkey.api.util.MailHelper; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.URLHelper; import org.labkey.api.util.CsrfInput; @@ -72,6 +80,7 @@ import org.springframework.validation.ObjectError; import org.springframework.web.servlet.ModelAndView; +import java.sql.SQLException; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -153,6 +162,8 @@ public boolean handlePost(AddPropertyForm addPropertyForm, BindException errors) } m.put(SignUpModule.SIGNUP_GROUP_NAME, addPropertyForm.getGroupName()); m.save(); + AuditLogService.get().addEvent(getUser(), + new ClientApiAuditProvider.ClientApiAuditEvent(c, "Signup target group set to '" + addPropertyForm.getGroupName() + "'.")); return true; } @@ -184,6 +195,8 @@ public boolean handlePost(ContainerIdForm containerIdForm, BindException errors) WritablePropertyMap m = PropertyManager.getWritableProperties(c, SignUpModule.SIGNUP_CATEGORY, true); m.remove(SignUpModule.SIGNUP_GROUP_NAME); m.save(); + AuditLogService.get().addEvent(getUser(), + new ClientApiAuditProvider.ClientApiAuditEvent(c, "Signup target group property removed.")); return true; } @@ -221,6 +234,10 @@ public boolean handlePost(AddGroupChangeForm addGroupChangeForm, BindException e m.put(String.valueOf(addGroupChangeForm.getOldgroup()), newProperties); m.save(); + AuditLogService.get().addEvent(getUser(), + new ClientApiAuditProvider.ClientApiAuditEvent(ContainerManager.getRoot(), + "Signup group-change rule added: members of group " + groupLabel(addGroupChangeForm.getOldgroup()) + + " may move to group " + groupLabel(addGroupChangeForm.getNewgroup()) + ".")); return true; } @@ -246,19 +263,34 @@ public boolean handlePost(AddGroupChangeForm addGroupChangeForm, BindException e int oldgroup = addGroupChangeForm.getOldgroup(); int newgroup = addGroupChangeForm.getNewgroup(); if(oldgroup == 0 || newgroup == 0) + { + errors.addError(new LabKeyError("Please select both a source and a target group.")); return false; + } WritablePropertyMap m = PropertyManager.getWritableProperties(SignUpModule.SIGNUP_GROUP_TO_GROUP, true); String existingRules = m.get(String.valueOf(oldgroup)); + if(existingRules == null) // no rules configured for this source group - nothing to remove + { + errors.addError(new LabKeyError("No group-change rule exists for the selected source group.")); + return false; + } ArrayList rules = new ArrayList<>(Arrays.asList(existingRules.split(","))); if(!rules.contains(String.valueOf(newgroup))) + { + errors.addError(new LabKeyError("No group-change rule exists from the selected source group to the selected target group.")); return false; + } rules.remove(String.valueOf(newgroup)); String newProperties = StringUtils.join(rules, ','); m.put(String.valueOf(oldgroup), newProperties); if(rules.isEmpty() || (rules.size() == 1 && rules.contains(""))) - m.remove(oldgroup); + m.remove(String.valueOf(oldgroup)); // keys are stored as String.valueOf(oldgroup) m.save(); + AuditLogService.get().addEvent(getUser(), + new ClientApiAuditProvider.ClientApiAuditEvent(ContainerManager.getRoot(), + "Signup group-change rule removed: members of group " + groupLabel(oldgroup) + + " may no longer move to group " + groupLabel(newgroup) + ".")); return true; } @@ -547,17 +579,9 @@ public ModelAndView getView(SignupForm form, boolean reshow, BindException error } else { - String message = form.isAccountExists() ? SignUpManager.USER_ALREADY_EXISTS : SignUpManager.CONFIRMATION_SENT; - message = String.format(message, form.getEmail()); - if(form.isAccountExists()) - { - errors.addError(new LabKeyError(message)); - return new SimpleErrorView(errors); - } - else - { - return HtmlView.of(message); - } + // Same response whether or not the account already exists (avoids user enumeration). + String message = String.format(SignUpManager.CONFIRMATION_SENT, form.getEmail()); + return HtmlView.of(message); } } @@ -583,25 +607,22 @@ public boolean handlePost(SignupForm signupForm, BindException errors) throws Ex return false; } - if (UserManager.userExists(email)) - { - // If the user already exists forward them to a page where they can click on a link to recover their password, if required - signupForm.setAccountExists(true); - signupForm.setNewSignUp(false); - return false; - } - try { - createUserAndSendEmail(signupForm, email); + // Do not reveal whether an account already exists (avoids user enumeration): both paths send + // an email and re-render the same "confirmation sent" message. Any failure is caught below and + // turned into the same generic error, so the outcome never depends on whether the account existed. + if (UserManager.userExists(email)) + sendExistingAccountEmail(email); // never modifies the existing account + else + createUserAndSendEmail(signupForm, email); } - catch (MessagingException | ConfigurationException e) + catch (MessagingException | ConfigurationException | SQLException e) { + // Same generic response for any of these failures, so the outcome never reveals whether the + // account existed. Logged server-side only, never leaked to the caller. + _log.error("Signup submission failed", e); errors.reject(ERROR_MSG, sendEmailErrorMessage(getContainer())); - if (e.getMessage() != null) - { - errors.reject(ERROR_MSG, e.getMessage()); - } return false; } @@ -719,6 +740,27 @@ private void createUserAndSendEmail(SignupForm form, ValidEmail email) } } + // Sends an informational email to the owner of an already-registered address when someone submits the + // signup form for that address. The existing account is never modified. A send failure propagates to the + // caller, which returns the same generic error as a failed new-signup send, so the response stays uniform + // whether or not the account exists (avoids user enumeration). + private void sendExistingAccountEmail(ValidEmail email) throws MessagingException + { + Container c = getContainer(); + String siteName = LookAndFeelProperties.getInstance(c).getShortName(); + ActionURL loginUrl = PageFlowUtil.urlProvider(LoginUrls.class).getLoginURL(c, null); + String body = "We received a request to create an account on " + siteName + + " using this email address. An account already exists for " + email.getEmailAddress() + ".\n\n" + + "If this was you and you have forgotten your password, go to the sign-in page and use " + + "the \"Forgot your password?\" link to reset it:\n" + loginUrl.getURIString() + "\n\n" + + "If you did not make this request, you can ignore this email."; + MailHelper.ViewMessage m = MailHelper.createMessage( + LookAndFeelProperties.getInstance(c).getSystemEmailAddress(), email.getEmailAddress()); + m.setSubject("You already have an account on " + siteName); + m.setText(body); + MailHelper.send(m, getUser(), c); + } + private static List errorsToMessages(Errors errors) { return errors.getAllErrors().stream() @@ -728,10 +770,51 @@ private static List errorsToMessages(Errors errors) private static String sendEmailErrorMessage(Container container) { - return "Could not send new user registration email. Please contact your server administrator at " + return "Could not send email. Please contact your server administrator at " + LookAndFeelProperties.getInstance(container).getSystemEmailAddress(); } + // Returns null if the requested self-service group change is allowed, otherwise a short reason. + // Both groups must be project groups in the same project, and the target must grant no more than + // read access. This bounds the damage if an admin maps a low-privilege group to a privileged, + // write-enabled, or site group in the transition rule map. + private static String validateGroupChangeTarget(Group oldgroup, Group newgroup) + { + if (oldgroup == null || newgroup == null) + return "source or target group no longer exists"; + if (!oldgroup.isProjectGroup() || !newgroup.isProjectGroup()) + return "site groups are not valid for self-service changes"; + if (!newgroup.getContainer().equals(oldgroup.getContainer())) + return "target group is in a different project than the source group"; + Container project = ContainerManager.getForId(newgroup.getContainer()); + if (project == null) + return "project no longer exists"; + // The self-service flow may only drop a user into a read-only group. Reject a target that carries + // write (Editor/Author/Submitter) or admin access anywhere in the project tree, so a misconfigured + // rule cannot be used to gain more than read access. + for (Container c : ContainerManager.getAllChildren(project)) + { + if (c.hasPermission(newgroup, AdminPermission.class) + || c.hasPermission(newgroup, InsertPermission.class) + || c.hasPermission(newgroup, UpdatePermission.class) + || c.hasPermission(newgroup, DeletePermission.class)) + return "target group grants more than read access"; + } + return null; + } + + // Renders a group as "name (id)" for audit messages. + private static String groupLabel(Group group) + { + return group.getName() + " (" + group.getUserId() + ")"; + } + + private static String groupLabel(int groupId) + { + Group group = SecurityManager.getGroup(groupId); + return group != null ? groupLabel(group) : "(" + groupId + ")"; + } + public static ActionURL getConfirmationURL(Container c, ValidEmail email, String key) { ActionURL url = new ActionURL(ConfirmAction.class, c); @@ -747,7 +830,6 @@ public static class SignupForm extends ReturnUrlForm private String _organization; private String _email; private String _emailConfirm; - private boolean _accountExists; private boolean _newSignUp = true; public String getFirstName() @@ -800,16 +882,6 @@ public void setEmailConfirm(String emailConfirm) _emailConfirm = emailConfirm; } - public boolean isAccountExists() - { - return _accountExists; - } - - public void setAccountExists(boolean accountExists) - { - _accountExists = accountExists; - } - public boolean isNewSignUp() { return _newSignUp; @@ -861,17 +933,34 @@ public ApiResponse execute(AddGroupChangeForm addGroupChangeForm, BindException response.put("status", "NO_PERMISSIONS"); return response; } - // once reached here we can assume that group a and b exist and that a rule exists allowing - // a user to change from group a to group b + // A matching rule exists, but the rule map is admin-configured and could point at a + // privileged target. Re-validate the resolved groups before mutating membership so a + // misconfigured rule cannot be used to self-escalate (see validateGroupChangeTarget). + final Group oldgroup = SecurityManager.getGroup(addGroupChangeForm.getOldgroup()); + final Group newgroup = SecurityManager.getGroup(addGroupChangeForm.getNewgroup()); + String denyReason = validateGroupChangeTarget(oldgroup, newgroup); + if (denyReason != null) + { + _log.warn("Rejected self-service group change for user {} (group {} -> {}): {}", + user.getEmail(), addGroupChangeForm.getOldgroup(), addGroupChangeForm.getNewgroup(), denyReason); + // A configured rule points at a target the self-service flow must never grant (privileged, + // write-enabled, site, or other-project). Return the same generic NO_PERMISSIONS as an + // ineligible caller so the response does not reveal which rules are misconfigured; the + // specific reason is logged server-side only, not returned to the caller. + response.put("status", "NO_PERMISSIONS"); + return response; + } try (DbScope.Transaction transaction = CoreSchema.getInstance().getSchema().getScope().ensureTransaction()) { - final Group oldgroup = SecurityManager.getGroup(addGroupChangeForm.getOldgroup()); - final Group newgroup = SecurityManager.getGroup(addGroupChangeForm.getNewgroup()); SecurityManager.addMember(newgroup, user); SecurityManager.deleteMember(oldgroup, user); UserManager.updateUser(user, user); addGroupChangeForm.setLabkeyUserId(user.getUserId()); Table.insert(user, SignUpSchema.getTableInfoMovedUsers(), addGroupChangeForm); + AuditLogService.get().addEvent(user, + new ClientApiAuditProvider.ClientApiAuditEvent(getContainer(), + "Self-service group change: user " + user.getEmail() + " moved from group " + + groupLabel(oldgroup) + " to group " + groupLabel(newgroup) + ".")); response.put("status", "USER_MOVED_SUCCESS"); // success status transaction.commit(); } @@ -914,32 +1003,29 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E return response; } - if (UserManager.userExists(email)) - { - response.put("status", "USER_EXISTS"); - return response; - } - try { - createUserAndSendEmail(signupForm, email); + // Do not reveal whether an account already exists (avoids user enumeration): both paths send + // an email and return the same response. Any failure is caught below and turned into the same + // generic ERROR, so the outcome never depends on whether the account existed. + if (UserManager.userExists(email)) + sendExistingAccountEmail(email); // never modifies the existing account + else + createUserAndSendEmail(signupForm, email); } - catch (MessagingException | ConfigurationException e) + catch (MessagingException | ConfigurationException | SQLException e) { + // Same generic response for any of these failures, so the outcome never reveals whether the + // account existed. Logged server-side only, never leaked to the caller. + _log.error("Signup submission failed", e); response.put("status", "ERROR"); - List messages = new ArrayList<>(); - messages.add(sendEmailErrorMessage(getContainer())); - if (e.getMessage() != null) - { - messages.add(e.getMessage()); - } - response.put("error_message", messages); + response.put("error_message", List.of(sendEmailErrorMessage(getContainer()))); return response; } clearCaptcha(); - response.put("status", "USER_ADDED"); + response.put("status", "SUCCESS"); return response; } } diff --git a/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java new file mode 100644 index 00000000..3f9fb517 --- /dev/null +++ b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java @@ -0,0 +1,367 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.test.tests.signup; + +import org.apache.hc.client5.http.classic.methods.HttpPost; +import org.apache.hc.client5.http.entity.UrlEncodedFormEntity; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse; +import org.apache.hc.core5.http.HttpStatus; +import org.apache.hc.core5.http.NameValuePair; +import org.apache.hc.core5.http.io.entity.EntityUtils; +import org.apache.hc.core5.http.message.BasicNameValuePair; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.labkey.remoteapi.CommandException; +import org.labkey.remoteapi.CommandResponse; +import org.labkey.remoteapi.Connection; +import org.labkey.remoteapi.SimplePostCommand; +import org.labkey.remoteapi.query.ContainerFilter; +import org.labkey.remoteapi.query.Filter; +import org.labkey.remoteapi.query.SelectRowsCommand; +import org.labkey.remoteapi.query.SelectRowsResponse; +import org.labkey.test.BaseWebDriverTest; +import org.labkey.test.WebTestHelper; +import org.labkey.test.categories.External; +import org.labkey.test.categories.MacCossLabModules; +import org.labkey.test.util.APITestHelper; +import org.labkey.test.util.ApiPermissionsHelper; +import org.labkey.test.util.LogMethod; +import org.labkey.test.util.PermissionsHelper.PrincipalType; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.labkey.test.util.PermissionsHelper.EDITOR_ROLE; +import static org.labkey.test.util.PermissionsHelper.FOLDER_ADMIN_ROLE; + +/** + * The self-service group change (ChangeGroupsApiAction) must not let a logged-in user escalate their own privileges. + * + * The transition rule map (the site-global "group A may move to group B" list) is admin-configured + * and could be pointed at a privileged target by mistake. ChangeGroupsApiAction therefore re-validates + * the resolved groups with validateGroupChangeTarget before it changes any membership, and refuses the + * move (status NO_PERMISSIONS) when the target: + * - is a site group rather than a project group, + * - is in a different project than the source group, or + * - grants more than read access (write or admin) anywhere in its project or its subfolders. + * + * This test plants each of those dangerous rules on purpose, then confirms that a non-admin member of + * the source group is refused every escalating move (and is neither added to the target nor removed from + * the source), while a legitimate move to a plain read-only project group in the same project still works. + * It also checks the audit log: a successful move is recorded as a "Client API Actions" event, and a + * refused move is not. + * + * A refused move and an ineligible caller both return the generic NO_PERMISSIONS so the response never + * reveals which configured rules are misconfigured; the specific reason is logged server-side only. Each + * escalating case still needs its transition rule planted so the attempt gets past the "rule exists" gate + * and actually reaches validateGroupChangeTarget (a target with no rule is refused earlier, also with + * NO_PERMISSIONS). The test drives a no-rule move on purpose as a contrast case. + * + * Design notes: + * - The live client (a skyline.ms wiki page) is not in the module, so this drives the server actions + * directly: the admin AddGroupChangeProperty action to plant rules, and the ChangeGroupsApi action as + * the user to attempt the moves. + * - The transition rule map is site-global (no container), so the planted rules are removed in @After + * (not doCleanup) so they are cleaned up even on a local clean=false run. + */ +@Category({External.class, MacCossLabModules.class}) +@BaseWebDriverTest.ClassTimeout(minutes = 2) +public class SignUpGroupChangeSecurityTest extends BaseWebDriverTest +{ + private static final String PROJECT_1 = "SignUpSecurityTest P1"; + private static final String PROJECT_2 = "SignUpSecurityTest P2"; + private static final String SUBFOLDER = "Sub"; + + private static final String GROUP_SOURCE = "SignupSource"; + private static final String GROUP_TARGET_OK = "SignupTargetOk"; + private static final String GROUP_TARGET_ADMIN = "SignupTargetAdmin"; + private static final String GROUP_TARGET_NESTED = "SignupTargetNested"; + private static final String GROUP_TARGET_SUBADMIN = "SignupTargetSubAdmin"; + private static final String GROUP_TARGET_CROSS = "SignupTargetCross"; + private static final String GROUP_TARGET_EDITOR = "SignupTargetEditor"; + private static final String SITE_GROUP = "SignupSiteGroup"; + + // Group ids captured during setup, used both for the rule map and for the ChangeGroupsApi params. + private static int idSource; + private static int idTargetOk; + private static int idTargetAdmin; + private static int idTargetNested; + private static int idTargetSubAdmin; + private static int idTargetCross; + private static int idTargetEditor; + private static int idSiteGroup; + + private static final String USER = "signup_user@signupsecurity.test"; + + // Password for the user account, so we can POST to ChangeGroupsApi as that user. + private static String userPassword; + + // Transition rules this test planted, so @After can remove them from the site-global rule map. + private final List _plantedRules = new ArrayList<>(); + + @Override + protected String getProjectName() + { + return PROJECT_1; + } + + @BeforeClass + public static void setupProject() + { + SignUpGroupChangeSecurityTest init = getCurrentTest(); + init.doSetup(); + } + + @LogMethod + private void doSetup() + { + _containerHelper.createProject(PROJECT_1, null); + _containerHelper.createSubfolder(PROJECT_1, SUBFOLDER); + _containerHelper.createProject(PROJECT_2, null); + + ApiPermissionsHelper perms = new ApiPermissionsHelper(this); + idSource = perms.createProjectGroup(GROUP_SOURCE, PROJECT_1); + idTargetOk = perms.createProjectGroup(GROUP_TARGET_OK, PROJECT_1); + idTargetAdmin = perms.createProjectGroup(GROUP_TARGET_ADMIN, PROJECT_1); + idTargetNested = perms.createProjectGroup(GROUP_TARGET_NESTED, PROJECT_1); + idTargetSubAdmin = perms.createProjectGroup(GROUP_TARGET_SUBADMIN, PROJECT_1); + idTargetCross = perms.createProjectGroup(GROUP_TARGET_CROSS, PROJECT_2); + idTargetEditor = perms.createProjectGroup(GROUP_TARGET_EDITOR, PROJECT_1); + idSiteGroup = perms.createGlobalPermissionsGroup(SITE_GROUP); + + // TargetAdmin carries admin in the P1 project; TargetSubAdmin carries admin only in the P1/Sub + // subfolder. Assigning a role in the subfolder gives it its own permission policy (breaks + // inheritance), which is what the subfolder branch of validateGroupChangeTarget looks for. + perms.addMemberToRole(idTargetAdmin, FOLDER_ADMIN_ROLE, "/" + PROJECT_1); + perms.addMemberToRole(idTargetSubAdmin, FOLDER_ADMIN_ROLE, "/" + PROJECT_1 + "/" + SUBFOLDER); + + // TargetEditor is a plain project group but is granted Editor (write) access in P1. The self-service + // flow may only move a user into a read-only group, so this write-enabled target must be refused too. + perms.addMemberToRole(idTargetEditor, EDITOR_ROLE, "/" + PROJECT_1); + + // TargetNested is a plain non-admin project group, but it is a MEMBER of TargetAdmin, so it inherits + // admin through nested group membership. validateGroupChangeTarget must catch this via hasPermission's + // group expansion, not only direct role assignments. (addUserToProjGroup adds the named group as a + // member when the name isn't a user.) + perms.addUserToProjGroup(GROUP_TARGET_NESTED, PROJECT_1, GROUP_TARGET_ADMIN); + + // A non-admin user who is a member of the source group. All rejection cases depend on the user + // being in Source, because the action checks source-group membership before it validates the target. + _userHelper.createUser(USER); + userPassword = setInitialPassword(USER); + perms.addUserToProjGroup(USER, PROJECT_1, GROUP_SOURCE); + } + + @Test + public void testSelfServiceGroupChangeRejectsPrivilegeEscalation() throws Exception + { + // Plant the four dangerous rules so each escalating attempt gets past the "rule exists" check and + // actually reaches validateGroupChangeTarget. TargetOk's rule is planted later, just before the + // happy path, so it can first stand in for the "no rule configured" case below. + addTransitionRule(idSource, idTargetAdmin); + addTransitionRule(idSource, idTargetNested); + addTransitionRule(idSource, idTargetSubAdmin); + addTransitionRule(idSource, idTargetCross); + addTransitionRule(idSource, idTargetEditor); + addTransitionRule(idSource, idSiteGroup); + + Connection userConnection = new Connection(WebTestHelper.getBaseURL(), USER, userPassword); + + // Contrast case: a valid same-project read-only target with no rule configured is refused before + // validateGroupChangeTarget even runs, with the same generic NO_PERMISSIONS. This pins the no-rule path. + assertMoveNotEligible(userConnection, idTargetOk, GROUP_TARGET_OK, + "no transition rule is configured for the target"); + + // Group transition validation rejections. Run these while the user is still in Source; each must + // leave membership untouched and report NO_PERMISSIONS. + assertMoveRejected(userConnection, idTargetAdmin, GROUP_TARGET_ADMIN, PROJECT_1, + "target group carries admin permission in its project"); + assertMoveRejected(userConnection, idTargetNested, GROUP_TARGET_NESTED, PROJECT_1, + "target group inherits admin via membership in an admin group"); + assertMoveRejected(userConnection, idTargetSubAdmin, GROUP_TARGET_SUBADMIN, PROJECT_1, + "target group carries admin permission in a subfolder"); + assertMoveRejected(userConnection, idTargetCross, GROUP_TARGET_CROSS, PROJECT_2, + "target group is in a different project"); + assertMoveRejected(userConnection, idTargetEditor, GROUP_TARGET_EDITOR, PROJECT_1, + "target group carries write (Editor) permission"); + assertMoveRejected(userConnection, idSiteGroup, SITE_GROUP, "/", + "target is a site group, not a project group"); + + // Legitimate move, run last because it actually moves the user out of the source group. + addTransitionRule(idSource, idTargetOk); + assertMoveSucceeded(userConnection, idTargetOk, GROUP_TARGET_OK); + } + + private void assertMoveRejected(Connection userConnection, int targetId, String targetGroup, + String targetContainer, String because) throws Exception + { + int auditBefore = countMoveAuditEvents(targetGroup); + String status = postGroupChange(userConnection, idSource, targetId); + assertEquals("Escalating move should be refused with NO_PERMISSIONS because " + because, + "NO_PERMISSIONS", status); + + ApiPermissionsHelper perms = new ApiPermissionsHelper(this); + assertFalse("User must not have been added to " + targetGroup + " (" + because + ")", + perms.isUserInGroup(USER, targetGroup, targetContainer, PrincipalType.USER)); + assertTrue("User must remain in the source group after a refused move (" + because + ")", + perms.isUserInGroup(USER, GROUP_SOURCE, PROJECT_1, PrincipalType.USER)); + assertEquals("A refused move to " + targetGroup + " must not add an audit event (" + because + ")", + auditBefore, countMoveAuditEvents(targetGroup)); + } + + // A move rejected before the group transition validation runs (e.g. no rule configured, or caller not in + // the source group) reports NO_PERMISSIONS -- the same generic status a validation rejection returns, so + // the response never distinguishes the two. This contrast case pins the no-rule path. + private void assertMoveNotEligible(Connection userConnection, int targetId, String targetGroup, + String because) throws Exception + { + int auditBefore = countMoveAuditEvents(targetGroup); + String status = postGroupChange(userConnection, idSource, targetId); + assertEquals("Move should be refused with NO_PERMISSIONS because " + because, "NO_PERMISSIONS", status); + + ApiPermissionsHelper perms = new ApiPermissionsHelper(this); + assertFalse("User must not have been added to " + targetGroup + " (" + because + ")", + perms.isUserInGroup(USER, targetGroup, PROJECT_1, PrincipalType.USER)); + assertTrue("User must remain in the source group after a refused move (" + because + ")", + perms.isUserInGroup(USER, GROUP_SOURCE, PROJECT_1, PrincipalType.USER)); + assertEquals("A refused move to " + targetGroup + " must not add an audit event (" + because + ")", + auditBefore, countMoveAuditEvents(targetGroup)); + } + + private void assertMoveSucceeded(Connection userConnection, int targetId, String targetGroup) throws Exception + { + int auditBefore = countMoveAuditEvents(targetGroup); + String status = postGroupChange(userConnection, idSource, targetId); + assertEquals("A legitimate move to a non-admin project group should succeed", "USER_MOVED_SUCCESS", status); + + ApiPermissionsHelper perms = new ApiPermissionsHelper(this); + assertTrue("User should now be a member of " + targetGroup, + perms.isUserInGroup(USER, targetGroup, PROJECT_1, PrincipalType.USER)); + assertFalse("User should have been removed from the source group", + perms.isUserInGroup(USER, GROUP_SOURCE, PROJECT_1, PrincipalType.USER)); + assertEquals("The successful move to " + targetGroup + " should add one audit event", + auditBefore + 1, countMoveAuditEvents(targetGroup)); + } + + // Counts existing audit-log entries for a self-service group change to targetGroup by this test's user. + // ChangeGroupsApiAction writes its "Client API Actions" event in the root container, which survives this + // test's project cleanup, so events from earlier runs remain. Callers take this count before and after a + // move and compare the two, so leftover events do not affect the result. + private int countMoveAuditEvents(String targetGroup) throws IOException, CommandException + { + SelectRowsCommand command = new SelectRowsCommand("auditLog", "Client API Actions"); + command.setColumns(List.of("Comment")); + command.setContainerFilter(ContainerFilter.AllFolders); + command.addFilter("Comment", USER, Filter.Operator.CONTAINS); + command.addFilter("Comment", targetGroup, Filter.Operator.CONTAINS); + SelectRowsResponse response = command.execute(createDefaultConnection(), "/"); + return response.getRows().size(); + } + + // Posts to ChangeGroupsApi as the user and returns the response status string. + private String postGroupChange(Connection userConnection, int oldGroup, int newGroup) throws Exception + { + SimplePostCommand command = new SimplePostCommand("signup", "changeGroupsApi"); + command.setParameters(Map.of("oldgroup", oldGroup, "newgroup", newGroup)); + CommandResponse response = command.execute(userConnection, "/"); + return (String) response.getProperty("status"); + } + + private void addTransitionRule(int oldGroup, int newGroup) throws IOException + { + postAdminGroupChangeForm("addGroupChangeProperty", oldGroup, newGroup); + _plantedRules.add(new int[]{oldGroup, newGroup}); + } + + // Drives the site-admin AddGroupChangeProperty / RemoveGroupChangeProperty form actions. These are + // FormHandlerActions (they redirect rather than return JSON), so this posts form-encoded fields as the + // logged-in admin: injectCookies carries the admin session and CSRF token, and getBasicHttpContext adds + // preemptive basic auth. + private void postAdminGroupChangeForm(String action, int oldGroup, int newGroup) throws IOException + { + HttpPost post = new HttpPost(WebTestHelper.buildURL("signup", "/", action)); + List form = List.of( + new BasicNameValuePair("oldgroup", String.valueOf(oldGroup)), + new BasicNameValuePair("newgroup", String.valueOf(newGroup))); + post.setEntity(new UrlEncodedFormEntity(form)); + APITestHelper.injectCookies(post); + + HttpContext context = WebTestHelper.getBasicHttpContext(); + try (CloseableHttpClient client = WebTestHelper.getHttpClient(); + CloseableHttpResponse response = client.execute(post, context)) + { + int code = response.getCode(); + assertTrue(action + " returned unexpected HTTP status " + code, + code == HttpStatus.SC_OK || code == HttpStatus.SC_MOVED_TEMPORARILY); + EntityUtils.consumeQuietly(response.getEntity()); + } + } + + @After + public void removePlantedRules() + { + // The transition rule map is site-global, so remove the rules this test planted even when project + // cleanup is skipped (clean=false). Runs after the test method. + if (_plantedRules.isEmpty()) + return; + + ensureSignedInAsPrimaryTestUser(); + for (int[] rule : _plantedRules) + { + try + { + postAdminGroupChangeForm("removeGroupChangeProperty", rule[0], rule[1]); + } + catch (IOException e) + { + log("Failed to remove planted transition rule " + rule[0] + " -> " + rule[1] + ": " + e.getMessage()); + } + } + _plantedRules.clear(); + } + + @Override + protected void doCleanup(boolean afterTest) + { + // The site group is not scoped to a project, so it survives project deletion and must be removed + // explicitly. + new ApiPermissionsHelper(this).deleteGroup(SITE_GROUP, false); + _userHelper.deleteUsers(false, USER); + _containerHelper.deleteProject(PROJECT_1, afterTest); + _containerHelper.deleteProject(PROJECT_2, afterTest); + } + + @Override + public List getAssociatedModules() + { + return List.of("signup"); + } + + @Override + protected BrowserType bestBrowser() + { + return BrowserType.CHROME; + } +} From 8797b6ca8c0789723cfed841f4fc78d6a842c91e Mon Sep 17 00:00:00 2001 From: Trey Chadick Date: Wed, 29 Jul 2026 10:33:57 -0700 Subject: [PATCH 2/2] Mark SignUpGroupChangeSecurityTest as postgres-only (#668) --- .../test/tests/signup/SignUpGroupChangeSecurityTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java index 3f9fb517..426b20e3 100644 --- a/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java +++ b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java @@ -44,6 +44,7 @@ import org.labkey.test.util.ApiPermissionsHelper; import org.labkey.test.util.LogMethod; import org.labkey.test.util.PermissionsHelper.PrincipalType; +import org.labkey.test.util.PostgresOnlyTest; import java.io.IOException; import java.util.ArrayList; @@ -88,7 +89,7 @@ */ @Category({External.class, MacCossLabModules.class}) @BaseWebDriverTest.ClassTimeout(minutes = 2) -public class SignUpGroupChangeSecurityTest extends BaseWebDriverTest +public class SignUpGroupChangeSecurityTest extends BaseWebDriverTest implements PostgresOnlyTest { private static final String PROJECT_1 = "SignUpSecurityTest P1"; private static final String PROJECT_2 = "SignUpSecurityTest P2";