diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 127febe0..1a18172b 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,13 @@ 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; 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 +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; @@ -153,6 +159,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 +192,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 +231,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; } @@ -249,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; @@ -256,9 +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 " + groupLabel(oldgroup) + + " may no longer move to group " + groupLabel(newgroup) + ".")); return true; } @@ -547,17 +567,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 +595,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 +728,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 +758,46 @@ 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 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 "project no longer exists"; + // Reject a target group with admin permission anywhere in the project. + for (Container c : ContainerManager.getAllChildren(project)) + { + if (c.hasPermission(newgroup, AdminPermission.class)) + return "target group carries administrative permission"; + } + 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 +813,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 +865,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 +916,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, + // 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()) { - 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 +986,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..94aae645 --- /dev/null +++ b/signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java @@ -0,0 +1,354 @@ +/* + * 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.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. + * 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 + * 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_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"; + + // 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 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); + 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); + + // 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, 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, 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, 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 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)); + 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, distinct from the validation's TARGET_NOT_ALLOWED. + 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; + } +}