From c5a296335697e52b50b6132799fb73d5515d1835 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Fri, 10 Jul 2026 17:55:29 -0700 Subject: [PATCH 1/8] Fix security issues in signup module. --- .../org/labkey/signup/SignUpController.java | 109 +++++++++++------- 1 file changed, 69 insertions(+), 40 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 127febe0..efdbb66c 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; @@ -52,6 +54,7 @@ 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.ReadPermission; import org.labkey.api.settings.LookAndFeelProperties; import org.labkey.api.util.ButtonBuilder; @@ -153,6 +156,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 +189,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 +228,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 " + addGroupChangeForm.getOldgroup() + + " may move to group " + addGroupChangeForm.getNewgroup() + ".")); return true; } @@ -259,6 +270,10 @@ public boolean handlePost(AddGroupChangeForm addGroupChangeForm, BindException e m.remove(oldgroup); m.save(); + AuditLogService.get().addEvent(getUser(), + new ClientApiAuditProvider.ClientApiAuditEvent(ContainerManager.getRoot(), + "Signup group-change rule removed: members of group " + oldgroup + + " may no longer move to group " + newgroup + ".")); return true; } @@ -547,17 +562,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); } } @@ -585,8 +592,9 @@ public boolean handlePost(SignupForm signupForm, BindException errors) throws Ex 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); + // Do not reveal whether an account already exists (avoids user enumeration). + // Show the same "confirmation sent" response as a new signup, without sending an email. + clearCaptcha(); signupForm.setNewSignUp(false); return false; } @@ -597,11 +605,10 @@ public boolean handlePost(SignupForm signupForm, BindException errors) throws Ex } catch (MessagingException | ConfigurationException e) { + // Log the underlying SMTP/configuration error server-side only; do not leak it to + // the (unauthenticated) caller. + _log.error("Failed to send signup confirmation email", e); errors.reject(ERROR_MSG, sendEmailErrorMessage(getContainer())); - if (e.getMessage() != null) - { - errors.reject(ERROR_MSG, e.getMessage()); - } return false; } @@ -732,6 +739,26 @@ private static String sendEmailErrorMessage(Container container) + LookAndFeelProperties.getInstance(container).getSystemEmailAddress(); } + // Returns null if the requested self-service group change is allowed, otherwise a short reason + // (for server-side logging). Both groups must be project groups in the same project, and the + // target must not carry admin rights. This bounds the damage if an admin maps a low-privilege + // group to a privileged 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 "target group's project no longer exists"; + if (project.hasPermission(newgroup, AdminPermission.class)) + return "target group carries administrative permission"; + return null; + } + public static ActionURL getConfirmationURL(Container c, ValidEmail email, String key) { ActionURL url = new ActionURL(ConfirmAction.class, c); @@ -747,7 +774,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 +826,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 +877,30 @@ 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); + 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 " + + oldgroup.getName() + " to group " + newgroup.getName() + ".")); response.put("status", "USER_MOVED_SUCCESS"); // success status transaction.commit(); } @@ -916,7 +945,10 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E if (UserManager.userExists(email)) { - response.put("status", "USER_EXISTS"); + // Do not reveal whether an account already exists (avoids user enumeration). + // Return the same response as a successful new signup, without sending an email. + clearCaptcha(); + response.put("status", "USER_ADDED"); return response; } @@ -926,14 +958,11 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E } catch (MessagingException | ConfigurationException e) { + // Log the underlying SMTP/configuration error server-side only; do not leak it to + // the (unauthenticated) caller. + _log.error("Failed to send signup confirmation email", 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; } From bd77688eb0e4013a2d7662c98e8d5373999d3ddc Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Thu, 23 Jul 2026 16:35:48 -0700 Subject: [PATCH 2/8] SignUpController.validateGroupChangeTarget now rejects a target group that carries AdminPermission (admin rights) in any subfolder of the project, not only at the project root. Co-Authored-By: Claude --- .../org/labkey/signup/SignUpController.java | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index efdbb66c..e3003f59 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -36,6 +36,7 @@ import org.labkey.api.audit.ClientApiAuditProvider; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; +import org.labkey.api.data.ContainerType; import org.labkey.api.data.CoreSchema; import org.labkey.api.data.DbScope; import org.labkey.api.data.PropertyManager; @@ -739,10 +740,10 @@ private static String sendEmailErrorMessage(Container container) + LookAndFeelProperties.getInstance(container).getSystemEmailAddress(); } - // Returns null if the requested self-service group change is allowed, otherwise a short reason - // (for server-side logging). Both groups must be project groups in the same project, and the - // target must not carry admin rights. This bounds the damage if an admin maps a low-privilege - // group to a privileged or site group in the transition rule map. + // 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 not carry admin rights. + // This bounds the damage if an admin maps a low-privilege group to a privileged or site group in the + // transition rule map. private static String validateGroupChangeTarget(Group oldgroup, Group newgroup) { if (oldgroup == null || newgroup == null) @@ -753,9 +754,16 @@ private static String validateGroupChangeTarget(Group oldgroup, Group newgroup) return "target group is in a different project than the source group"; Container project = ContainerManager.getForId(newgroup.getContainer()); if (project == null) - return "target group's project no longer exists"; - if (project.hasPermission(newgroup, AdminPermission.class)) - return "target group carries administrative permission"; + return "project no longer exists"; + // Check the project and subfolders for admin permission. Skip folders that inherit + // their parent's policy - they have no assignment of their own to catch. + for (Container c : ContainerManager.getAllChildren(project)) + { + if (!c.isContainerFor(ContainerType.DataType.permissions)) + continue; + if (c.hasPermission(newgroup, AdminPermission.class)) + return "target group carries administrative permission"; + } return null; } From ee4451e3dfa682c124dc5718eb676cd6c62a5505 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Fri, 24 Jul 2026 13:27:16 -0700 Subject: [PATCH 3/8] Emailed the account owner when signup hits an existing address * SignUpApiAction and BeginAction now email the owner of an already-registered address (new sendExistingAccountEmail helper) instead of doing nothing. Both signup paths now send an email, so response timing no longer reveals whether an account exists. * The existing account is never modified, and send failures are logged and swallowed so the response stays identical to a new signup. The email points the owner to the "Forgot your password?" reset flow. * Renamed the SignUpApiAction success status from USER_ADDED to SUCCESS, which was misleading in the existing-account case where no user is added. The external signup wiki form must key its success branch off SUCCESS. Co-Authored-By: Claude --- .../org/labkey/signup/SignUpController.java | 41 +++++++++++++++++-- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index e3003f59..4da198dd 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -47,6 +47,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; @@ -61,6 +62,7 @@ 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; @@ -594,7 +596,8 @@ public boolean handlePost(SignupForm signupForm, BindException errors) throws Ex if (UserManager.userExists(email)) { // Do not reveal whether an account already exists (avoids user enumeration). - // Show the same "confirmation sent" response as a new signup, without sending an email. + // Notify the real owner and show the same "confirmation sent" response as a new signup. + sendExistingAccountEmail(email); clearCaptcha(); signupForm.setNewSignUp(false); return false; @@ -727,6 +730,35 @@ 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. Both the new-signup and existing-account paths + // now send an email. The existing account is never modified. Send failures are logged and + // swallowed so the caller still returns the same success response as a new signup. + private void sendExistingAccountEmail(ValidEmail email) + { + Container c = getContainer(); + try + { + 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); + } + catch (Exception e) + { + // Log and continue so the response stays identical to the new-signup case. + _log.error("Failed to send existing-account email", e); + } + } + private static List errorsToMessages(Errors errors) { return errors.getAllErrors().stream() @@ -954,9 +986,10 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E if (UserManager.userExists(email)) { // Do not reveal whether an account already exists (avoids user enumeration). - // Return the same response as a successful new signup, without sending an email. + // Notify the account owner and return the same response as a successful new signup. + sendExistingAccountEmail(email); clearCaptcha(); - response.put("status", "USER_ADDED"); + response.put("status", "SUCCESS"); return response; } @@ -976,7 +1009,7 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E clearCaptcha(); - response.put("status", "USER_ADDED"); + response.put("status", "SUCCESS"); return response; } } From f601fe8650241200c13ec766850182fb4504350a Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Fri, 24 Jul 2026 15:52:36 -0700 Subject: [PATCH 4/8] 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 --- .../org/labkey/signup/SignUpController.java | 7 +- .../signup/SignUpGroupChangeSecurityTest.java | 312 ++++++++++++++++++ 2 files changed, 318 insertions(+), 1 deletion(-) 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 4da198dd..55bb353e 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -794,6 +794,7 @@ private static String validateGroupChangeTarget(Group oldgroup, Group newgroup) if (!c.isContainerFor(ContainerType.DataType.permissions)) continue; if (c.hasPermission(newgroup, AdminPermission.class)) + return "target group carries administrative permission"; } return null; @@ -927,7 +928,11 @@ public ApiResponse execute(AddGroupChangeForm addGroupChangeForm, BindException { _log.warn("Rejected self-service group change for user {} (group {} -> {}): {}", user.getEmail(), addGroupChangeForm.getOldgroup(), addGroupChangeForm.getNewgroup(), denyReason); - response.put("status", "NO_PERMISSIONS"); + // A configured rule points at a target the self-service flow must never grant (privileged, + // site, or other-project). Report this with its own status so a refused misconfiguration is + // distinguishable from an ineligible caller (NO_PERMISSIONS above). The specific reason is + // logged server-side only, not returned to the caller. + response.put("status", "TARGET_NOT_ALLOWED"); return response; } try (DbScope.Transaction transaction = CoreSchema.getInstance().getSchema().getScope().ensureTransaction()) 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..7d8301af --- /dev/null +++ b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java @@ -0,0 +1,312 @@ +/* + * 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.CommandResponse; +import org.labkey.remoteapi.Connection; +import org.labkey.remoteapi.SimplePostCommand; +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.MemberType; +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.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 rejects the + * move with status TARGET_NOT_ALLOWED when the target: + * - is a site group rather than a project group, + * - is in a different project than the source group, or + * - carries administrative permission 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 non-admin project group in the same project still works. + * + * Each escalating case 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 before that check, with + * NO_PERMISSIONS. The test drives a no-rule move on purpose and asserts NO_PERMISSIONS, so that path stays + * distinguishable from the TARGET_NOT_ALLOWED path. + * + * 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_SUBADMIN = "SignupTargetSubAdmin"; + private static final String GROUP_TARGET_CROSS = "SignupTargetCross"; + 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 idTargetSubAdmin; + private static int idTargetCross; + 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); + idTargetSubAdmin = perms.createProjectGroup(GROUP_TARGET_SUBADMIN, PROJECT_1); + idTargetCross = perms.createProjectGroup(GROUP_TARGET_CROSS, PROJECT_2); + 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); + + // 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, idTargetSubAdmin); + addTransitionRule(idSource, idTargetCross); + addTransitionRule(idSource, idSiteGroup); + + Connection userConnection = new Connection(WebTestHelper.getBaseURL(), USER, userPassword); + + // Contrast case: a valid same-project non-admin target with no rule configured is rejected before + // validateGroupChangeTarget, with NO_PERMISSIONS. Asserting this keeps the two rejection reasons distinguishable. + 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 TARGET_NOT_ALLOWED. + assertMoveRejected(userConnection, idTargetAdmin, GROUP_TARGET_ADMIN, PROJECT_1, + "target group carries admin permission in its project"); + 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, 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 + { + String status = postGroupChange(userConnection, idSource, targetId); + assertEquals("Escalating move should be refused with TARGET_NOT_ALLOWED because " + because, + "TARGET_NOT_ALLOWED", 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)); + } + + // A move rejected before the group transition validation runs (e.g. no rule configured, or caller not in + // the source group) reports NO_PERMISSIONS, distinct from the validation's TARGET_NOT_ALLOWED. + private void assertMoveNotEligible(Connection userConnection, int targetId, String targetGroup, + String because) throws Exception + { + 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)); + } + + private void assertMoveSucceeded(Connection userConnection, int targetId, String targetGroup) throws Exception + { + 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)); + } + + // 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 d6caf09d9fd9645974bbf72c4d8471540b685328 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Fri, 24 Jul 2026 17:33:05 -0700 Subject: [PATCH 5/8] Hardened the signup enumeration response and added audit checks to the group-change test * SignUpApiAction and BeginAction now run the existing-account and new-account branches through one try/catch, so a mail-send failure returns the same generic error for both. * SignUpGroupChangeSecurityTest now verifies through the auditLog schema that a successful self-service group change adds one "Client API Actions" event and a refused move adds none, comparing counts before and after each move since the event is recorded in the root container that outlives project cleanup. See ai/todos/active/TODO-LK-20260723_signup-security.md Co-Authored-By: Claude --- .../org/labkey/signup/SignUpController.java | 84 ++++++++----------- .../signup/SignUpGroupChangeSecurityTest.java | 31 +++++++ 2 files changed, 65 insertions(+), 50 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 55bb353e..12e99508 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -593,25 +593,21 @@ public boolean handlePost(SignupForm signupForm, BindException errors) throws Ex return false; } - if (UserManager.userExists(email)) - { - // Do not reveal whether an account already exists (avoids user enumeration). - // Notify the real owner and show the same "confirmation sent" response as a new signup. - sendExistingAccountEmail(email); - clearCaptcha(); - 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. On a send failure both reject + // with 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) { // Log the underlying SMTP/configuration error server-side only; do not leak it to // the (unauthenticated) caller. - _log.error("Failed to send signup confirmation email", e); + _log.error("Failed to send signup email", e); errors.reject(ERROR_MSG, sendEmailErrorMessage(getContainer())); return false; } @@ -730,33 +726,25 @@ 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. Both the new-signup and existing-account paths - // now send an email. The existing account is never modified. Send failures are logged and - // swallowed so the caller still returns the same success response as a new signup. - private void sendExistingAccountEmail(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(); - try - { - 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); - } - catch (Exception e) - { - // Log and continue so the response stays identical to the new-signup case. - _log.error("Failed to send existing-account email", e); - } + 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) @@ -768,7 +756,7 @@ 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(); } @@ -988,25 +976,21 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E return response; } - if (UserManager.userExists(email)) - { - // Do not reveal whether an account already exists (avoids user enumeration). - // Notify the account owner and return the same response as a successful new signup. - sendExistingAccountEmail(email); - clearCaptcha(); - response.put("status", "SUCCESS"); - 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. On a send failure both return 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) { // Log the underlying SMTP/configuration error server-side only; do not leak it to // the (unauthenticated) caller. - _log.error("Failed to send signup confirmation email", e); + _log.error("Failed to send signup email", e); response.put("status", "ERROR"); response.put("error_message", List.of(sendEmailErrorMessage(getContainer()))); 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 index 7d8301af..ba801a45 100644 --- a/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java +++ b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java @@ -28,9 +28,14 @@ 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; @@ -65,6 +70,8 @@ * 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 non-admin 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. * * Each escalating case 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 before that check, with @@ -187,6 +194,7 @@ public void testSelfServiceGroupChangeRejectsPrivilegeEscalation() throws Except 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 TARGET_NOT_ALLOWED because " + because, "TARGET_NOT_ALLOWED", status); @@ -196,6 +204,8 @@ private void assertMoveRejected(Connection userConnection, int targetId, String 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 @@ -203,6 +213,7 @@ private void assertMoveRejected(Connection userConnection, int targetId, String 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); @@ -211,10 +222,13 @@ private void assertMoveNotEligible(Connection userConnection, int targetId, Stri 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); @@ -223,6 +237,23 @@ private void assertMoveSucceeded(Connection userConnection, int targetId, String 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. From 9be75ff6eb2fe9a1050db6914d1cea530204ae31 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sun, 26 Jul 2026 13:18:55 -0700 Subject: [PATCH 6/8] Addressed code-review findings on the signup security changes * SignUpApiAction and BeginAction also catch SQLException from the new-account insert, so a DB failure returns the same generic error as any other and cannot reveal whether the account existed. * validateGroupChangeTarget drops the workbook-only container filter and its stale comment. The admin check is unchanged. * RemoveGroupChangeProperty now guards against a null rule map and removes the entry with the correct string key. Group-change audit events log group names instead of bare ids. * SignUpGroupChangeSecurityTest adds a case for a target that inherits admin via membership in an admin group. Co-Authored-By: Claude --- .../org/labkey/signup/SignUpController.java | 51 ++++++++++--------- .../signup/SignUpGroupChangeSecurityTest.java | 12 +++++ 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 12e99508..c2094d27 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -36,7 +36,6 @@ import org.labkey.api.audit.ClientApiAuditProvider; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; -import org.labkey.api.data.ContainerType; import org.labkey.api.data.CoreSchema; import org.labkey.api.data.DbScope; import org.labkey.api.data.PropertyManager; @@ -78,6 +77,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; @@ -233,8 +233,8 @@ public boolean handlePost(AddGroupChangeForm addGroupChangeForm, BindException e m.save(); AuditLogService.get().addEvent(getUser(), new ClientApiAuditProvider.ClientApiAuditEvent(ContainerManager.getRoot(), - "Signup group-change rule added: members of group " + addGroupChangeForm.getOldgroup() - + " may move to group " + addGroupChangeForm.getNewgroup() + ".")); + "Signup group-change rule added: members of group " + groupLabel(addGroupChangeForm.getOldgroup()) + + " may move to group " + groupLabel(addGroupChangeForm.getNewgroup()) + ".")); return true; } @@ -263,6 +263,8 @@ public boolean handlePost(AddGroupChangeForm addGroupChangeForm, BindException e 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 + return false; ArrayList rules = new ArrayList<>(Arrays.asList(existingRules.split(","))); if(!rules.contains(String.valueOf(newgroup))) return false; @@ -270,13 +272,13 @@ public boolean handlePost(AddGroupChangeForm addGroupChangeForm, BindException e 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 " + oldgroup - + " may no longer move to group " + newgroup + ".")); + "Signup group-change rule removed: members of group " + groupLabel(oldgroup) + + " may no longer move to group " + groupLabel(newgroup) + ".")); return true; } @@ -596,18 +598,18 @@ public boolean handlePost(SignupForm signupForm, BindException errors) throws Ex try { // Do not reveal whether an account already exists (avoids user enumeration): both paths send - // an email and re-render the same "confirmation sent" message. On a send failure both reject - // with the same generic error, so the outcome never depends on whether the account existed. + // 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) { - // Log the underlying SMTP/configuration error server-side only; do not leak it to - // the (unauthenticated) caller. - _log.error("Failed to send signup email", 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())); return false; } @@ -775,19 +777,22 @@ private static String validateGroupChangeTarget(Group oldgroup, Group newgroup) Container project = ContainerManager.getForId(newgroup.getContainer()); if (project == null) return "project no longer exists"; - // Check the project and subfolders for admin permission. Skip folders that inherit - // their parent's policy - they have no assignment of their own to catch. + // Reject a target group with admin permission anywhere in the project. for (Container c : ContainerManager.getAllChildren(project)) { - if (!c.isContainerFor(ContainerType.DataType.permissions)) - continue; if (c.hasPermission(newgroup, AdminPermission.class)) - return "target group carries administrative permission"; } return null; } + // Renders a group id as "name (id)" for audit messages, falling back to just the id if the group is gone. + private static String groupLabel(int groupId) + { + Group group = SecurityManager.getGroup(groupId); + return group != null ? group.getName() + " (" + groupId + ")" : "(" + groupId + ")"; + } + public static ActionURL getConfirmationURL(Container c, ValidEmail email, String key) { ActionURL url = new ActionURL(ConfirmAction.class, c); @@ -979,18 +984,18 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E try { // Do not reveal whether an account already exists (avoids user enumeration): both paths send - // an email and return the same response. On a send failure both return the same generic ERROR, - // so the outcome never depends on whether the account existed. + // 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) { - // Log the underlying SMTP/configuration error server-side only; do not leak it to - // the (unauthenticated) caller. - _log.error("Failed to send signup email", 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"); response.put("error_message", List.of(sendEmailErrorMessage(getContainer()))); 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 index ba801a45..e40d6738 100644 --- a/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java +++ b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java @@ -96,6 +96,7 @@ public class SignUpGroupChangeSecurityTest extends BaseWebDriverTest 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 SITE_GROUP = "SignupSiteGroup"; @@ -104,6 +105,7 @@ public class SignUpGroupChangeSecurityTest extends BaseWebDriverTest 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 idSiteGroup; @@ -140,6 +142,7 @@ private void doSetup() 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); idSiteGroup = perms.createGlobalPermissionsGroup(SITE_GROUP); @@ -150,6 +153,12 @@ private void doSetup() perms.addMemberToRole(idTargetAdmin, FOLDER_ADMIN_ROLE, "/" + PROJECT_1); perms.addMemberToRole(idTargetSubAdmin, FOLDER_ADMIN_ROLE, "/" + PROJECT_1 + "/" + SUBFOLDER); + // 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); @@ -164,6 +173,7 @@ public void testSelfServiceGroupChangeRejectsPrivilegeEscalation() throws Except // 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, idSiteGroup); @@ -179,6 +189,8 @@ public void testSelfServiceGroupChangeRejectsPrivilegeEscalation() throws Except // leave membership untouched and report TARGET_NOT_ALLOWED. 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, From 9dbfa81ca8e1c26af2392334ac6fffe15137ee02 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sun, 26 Jul 2026 15:39:14 -0700 Subject: [PATCH 7/8] Removed unused import --- .../labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java | 1 - 1 file changed, 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 e40d6738..94aae645 100644 --- a/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java +++ b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java @@ -43,7 +43,6 @@ import org.labkey.test.util.APITestHelper; import org.labkey.test.util.ApiPermissionsHelper; import org.labkey.test.util.LogMethod; -import org.labkey.test.util.PermissionsHelper.MemberType; import org.labkey.test.util.PermissionsHelper.PrincipalType; import java.io.IOException; From 79195172dbb07ebdbefcff3fe9b9f7fdb8824a19 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sun, 26 Jul 2026 21:14:04 -0700 Subject: [PATCH 8/8] SignUpController: added group ids to the self-service group-change audit message, matching the admin-config audit events --- signup/src/org/labkey/signup/SignUpController.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index c2094d27..1a18172b 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -786,11 +786,16 @@ private static String validateGroupChangeTarget(Group oldgroup, Group newgroup) return null; } - // Renders a group id as "name (id)" for audit messages, falling back to just the id if the group is gone. + // 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 ? group.getName() + " (" + groupId + ")" : "(" + groupId + ")"; + return group != null ? groupLabel(group) : "(" + groupId + ")"; } public static ActionURL getConfirmationURL(Container c, ValidEmail email, String key) @@ -938,7 +943,7 @@ public ApiResponse execute(AddGroupChangeForm addGroupChangeForm, BindException AuditLogService.get().addEvent(user, new ClientApiAuditProvider.ClientApiAuditEvent(getContainer(), "Self-service group change: user " + user.getEmail() + " moved from group " - + oldgroup.getName() + " to group " + newgroup.getName() + ".")); + + groupLabel(oldgroup) + " to group " + groupLabel(newgroup) + ".")); response.put("status", "USER_MOVED_SUCCESS"); // success status transaction.commit(); }