From 9aef65807d9e2e6bb95567afdff9fd95daaee4f1 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Wed, 20 May 2026 18:47:24 -0700 Subject: [PATCH 1/4] Added kaptcha verification to SignUpApiAction * Added kaptchaText field to SignupForm * SignUpController.SignUpApiAction.execute() now checks the session kaptcha value, rejects mismatches, and clears the attribute on success * Populated error_message in the JSON response for invalid-email and validateSignupForm error paths so the wiki form can display them See ai/todos/active/TODO-LK-20260517_add-kaptcha-to-signup.md Co-Authored-By: Claude --- .../org/labkey/signup/SignUpController.java | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index a81f690a..5ee1764a 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -62,6 +62,7 @@ import org.labkey.api.view.HtmlView; import org.labkey.api.view.HttpView; import org.labkey.api.view.JspView; +import org.labkey.api.view.LabKeyKaptchaServlet; import org.labkey.api.view.NavTree; import org.labkey.api.view.WebPartView; import org.springframework.validation.BindException; @@ -719,6 +720,18 @@ public void setNewSignUp(boolean newSignUp) { _newSignUp = newSignUp; } + + private String _kaptchaText; + + public String getKaptchaText() + { + return _kaptchaText; + } + + public void setKaptchaText(String kaptchaText) + { + _kaptchaText = kaptchaText; + } } @RequiresLogin @@ -778,6 +791,22 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E { ApiSimpleResponse response = new ApiSimpleResponse(); + String expectedKaptcha = (String) getViewContext().getRequest().getSession(true) + .getAttribute(LabKeyKaptchaServlet.SESSION_KEY_VALUE); + if (expectedKaptcha == null) + { + _log.info("Captcha not initialized for signup attempt"); + response.put("error_message", List.of("Captcha not initialized, please retry.")); + return response; + } + if (!expectedKaptcha.equalsIgnoreCase(StringUtils.trimToNull(signupForm.getKaptchaText()))) + { + _log.warn("Captcha text did not match for signup attempt for " + signupForm.getEmail()); + response.put("error_message", List.of("Verification text does not match, please retry.")); + return response; + } + getViewContext().getRequest().getSession(true).removeAttribute(LabKeyKaptchaServlet.SESSION_KEY_VALUE); + ValidEmail email; try { @@ -785,7 +814,7 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E } catch (ValidEmail.InvalidEmailException iee) { - errors.reject(ERROR_MSG, iee.getMessage()); + response.put("error_message", List.of(iee.getMessage())); return response; } @@ -798,6 +827,10 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E validateSignupForm(signupForm, errors); if(errors.hasErrors()) { + List messages = errors.getAllErrors().stream() + .map(e -> e.getDefaultMessage()) + .toList(); + response.put("error_message", messages); return response; } From c79cf0e6c3268c506a04d69d357c8d87a667b152 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Wed, 20 May 2026 21:48:38 -0700 Subject: [PATCH 2/4] Unified BeginAction and SignUpApiAction signup paths * Extracted verifyCaptcha, parseAndValidateEmail, sendConfirmationEmail helpers; both actions now run the same sequence (captcha -> blank fields -> email format -> userExists -> send) * Added emailConfirm field to SignupForm; validateSignupForm now also checks blank + match * sendConfirmationEmail wraps the TempUser insert + sendEmail in a DbScope transaction so the row rolls back on send failure * SignUpApiAction now also runs EmailValidator.isValid (closes a real divergence from BeginAction) and no longer falls through to status=USER_ADDED after a sendEmail failure * Rewrote signupPage.jsp using / with Confirm Email and kaptcha sections See ai/todos/active/TODO-LK-20260517_add-kaptcha-to-signup.md Co-Authored-By: Claude --- .../org/labkey/signup/SignUpController.java | 202 +++++++++++------- signup/src/org/labkey/signup/signupPage.jsp | 55 +++-- 2 files changed, 156 insertions(+), 101 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 5ee1764a..5555280f 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -16,6 +16,7 @@ package org.labkey.signup; +import jakarta.mail.MessagingException; import org.apache.commons.lang3.StringUtils; import org.apache.commons.validator.routines.EmailValidator; import org.apache.logging.log4j.LogManager; @@ -528,7 +529,8 @@ public class BeginAction extends FormViewAction @Override public void validateCommand(SignupForm form, Errors errors) { - validateSignupForm(form, errors); + // All validation happens in handlePost so the order matches SignUpApiAction + // (captcha first, then blank-field checks, then email parsing, etc.). } @Override @@ -560,27 +562,26 @@ public ModelAndView getView(SignupForm form, boolean reshow, BindException error @Override public boolean handlePost(SignupForm signupForm, BindException errors) throws Exception { - // Validate with EmailValidator first. ValidEmail(email) will not throw an exception if the domain is - // missing from the email. The default domain configured for the server is appended. - EmailValidator validator = EmailValidator.getInstance(); - if(!validator.isValid(signupForm.getEmail())) + String kaptchaError = verifyCaptcha(signupForm.getKaptchaText(), signupForm.getEmail()); + if (kaptchaError != null) { - errors.reject(ERROR_MSG,"'" + signupForm.getEmail() + "' is not a valid email address."); + errors.reject(ERROR_MSG, kaptchaError); return false; } - ValidEmail email; - try + validateSignupForm(signupForm, errors); + if (errors.hasErrors()) { - email = new ValidEmail(signupForm.getEmail()); + return false; } - catch (ValidEmail.InvalidEmailException iee) + + ValidEmail email = parseAndValidateEmail(signupForm, errors); + if (email == null) { - errors.reject(ERROR_MSG, iee.getMessage()); return false; } - if(UserManager.userExists(email)) + 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); @@ -588,26 +589,12 @@ public boolean handlePost(SignupForm signupForm, BindException errors) throws Ex return false; } - // If the user does not exit in LabKey's core database, check in our temporaryusers table - TempUser tempUser = getTempUser(signupForm, email); - - // Send email to the user. - ActionURL confirmationUrl = getConfirmationURL(getContainer(), email, tempUser.getKey()); - try + if (!createUserAndSendEmail(signupForm, email, errors)) { - User mockUser = new User(); - mockUser.setEmail(email.getEmailAddress()); - SecurityManager.sendEmail(getContainer(), mockUser, SecurityManager.getRegistrationMessage(null, false), email.getEmailAddress(), confirmationUrl); - } - catch(Exception e) - { - String systemEmail = LookAndFeelProperties.getInstance(getContainer()).getSystemEmailAddress(); - errors.reject(ERROR_MSG, "Could not send new user registration email. Please contact your server administrator at " + systemEmail); - errors.reject(ERROR_MSG, e.getMessage()); return false; } - + // Re-render the JSP with the CONFIRMATION_SENT message. signupForm.setNewSignUp(false); return false; } @@ -642,6 +629,96 @@ private void validateSignupForm(SignupForm form, Errors errors) { errors.reject(ERROR_MSG, "Email cannot be blank."); } + if(StringUtils.isBlank(form.getEmailConfirm())) + { + errors.reject(ERROR_MSG, "Confirm email cannot be blank."); + } + else if(form.getEmail() != null && !form.getEmail().equals(form.getEmailConfirm())) + { + errors.reject(ERROR_MSG, "Email addresses do not match."); + } + } + + // On success returns null and clears the session attribute so the captcha cannot be replayed. + // On failure return a a user-facing error message. + // Logging matches LoginController's RegisterUserAction. + private String verifyCaptcha(String submittedText, String emailForLogging) + { + var session = getViewContext().getRequest().getSession(true); + String expected = (String) session.getAttribute(LabKeyKaptchaServlet.SESSION_KEY_VALUE); + if (expected == null) + { + _log.info("Captcha not initialized for signup attempt"); + return "Captcha not initialized, please retry."; + } + if (!expected.equalsIgnoreCase(StringUtils.trimToNull(submittedText))) + { + _log.warn("Captcha text did not match for signup attempt for " + emailForLogging); + return "Verification text does not match, please retry."; + } + session.removeAttribute(LabKeyKaptchaServlet.SESSION_KEY_VALUE); + return null; + } + + // Returns a parsed ValidEmail, or null if the address is invalid (errors populated). + // Uses EmailValidator first because ValidEmail's constructor does not throw on bare + // strings like "foo" - it silently appends the server's default domain. + private ValidEmail parseAndValidateEmail(SignupForm form, Errors errors) + { + EmailValidator validator = EmailValidator.getInstance(); + if (!validator.isValid(form.getEmail())) + { + errors.reject(ERROR_MSG, "'" + form.getEmail() + "' is not a valid email address."); + return null; + } + try + { + return new ValidEmail(form.getEmail()); + } + catch (ValidEmail.InvalidEmailException iee) + { + errors.reject(ERROR_MSG, iee.getMessage()); + return null; + } + } + + // Creates (or reuses) a TempUser row and sends the confirmation email in a single + // transaction. On send failure the transaction is rolled back so a freshly inserted + // TempUser row does not persist when the user never received a confirmation link. + private boolean createUserAndSendEmail(SignupForm form, ValidEmail email, Errors errors) throws java.sql.SQLException + { + try (DbScope.Transaction transaction = SignUpManager.getSchema().getScope().ensureTransaction()) + { + TempUser tempUser = getTempUser(form, email); + ActionURL confirmationUrl = getConfirmationURL(getContainer(), email, tempUser.getKey()); + try + { + User mockUser = new User(); + mockUser.setEmail(email.getEmailAddress()); + SecurityManager.sendEmail(getContainer(), mockUser, + SecurityManager.getRegistrationMessage(null, false), + email.getEmailAddress(), confirmationUrl); + } + catch (MessagingException e) + { + String systemEmail = LookAndFeelProperties.getInstance(getContainer()).getSystemEmailAddress(); + errors.reject(ERROR_MSG, "Could not send new user registration email. Please contact your server administrator at " + systemEmail); + if (e.getMessage() != null) + { + errors.reject(ERROR_MSG, e.getMessage()); + } + return false; + } + transaction.commit(); + return true; + } + } + + private static List errorsToMessages(Errors errors) + { + return errors.getAllErrors().stream() + .map(e -> e.getDefaultMessage()) + .toList(); } public static ActionURL getConfirmationURL(Container c, ValidEmail email, String key) @@ -658,6 +735,7 @@ public static class SignupForm extends ReturnUrlForm private String _lastName; private String _organization; private String _email; + private String _emailConfirm; private boolean _accountExists; private boolean _newSignUp = true; @@ -701,6 +779,16 @@ public void setEmail(String email) _email = email; } + public String getEmailConfirm() + { + return _emailConfirm; + } + + public void setEmailConfirm(String emailConfirm) + { + _emailConfirm = emailConfirm; + } + public boolean isAccountExists() { return _accountExists; @@ -791,72 +879,40 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E { ApiSimpleResponse response = new ApiSimpleResponse(); - String expectedKaptcha = (String) getViewContext().getRequest().getSession(true) - .getAttribute(LabKeyKaptchaServlet.SESSION_KEY_VALUE); - if (expectedKaptcha == null) + String kaptchaError = verifyCaptcha(signupForm.getKaptchaText(), signupForm.getEmail()); + if (kaptchaError != null) { - _log.info("Captcha not initialized for signup attempt"); - response.put("error_message", List.of("Captcha not initialized, please retry.")); + response.put("error_message", List.of(kaptchaError)); return response; } - if (!expectedKaptcha.equalsIgnoreCase(StringUtils.trimToNull(signupForm.getKaptchaText()))) + + validateSignupForm(signupForm, errors); + if (errors.hasErrors()) { - _log.warn("Captcha text did not match for signup attempt for " + signupForm.getEmail()); - response.put("error_message", List.of("Verification text does not match, please retry.")); + response.put("error_message", errorsToMessages(errors)); return response; } - getViewContext().getRequest().getSession(true).removeAttribute(LabKeyKaptchaServlet.SESSION_KEY_VALUE); - ValidEmail email; - try - { - email = new ValidEmail(signupForm.getEmail()); - } - catch (ValidEmail.InvalidEmailException iee) + ValidEmail email = parseAndValidateEmail(signupForm, errors); + if (email == null) { - response.put("error_message", List.of(iee.getMessage())); + response.put("error_message", errorsToMessages(errors)); return response; } - if(UserManager.userExists(email)) + if (UserManager.userExists(email)) { response.put("status", "USER_EXISTS"); return response; } - validateSignupForm(signupForm, errors); - if(errors.hasErrors()) + if (!createUserAndSendEmail(signupForm, email, errors)) { - List messages = errors.getAllErrors().stream() - .map(e -> e.getDefaultMessage()) - .toList(); - response.put("error_message", messages); + response.put("error_message", errorsToMessages(errors)); return response; } - TempUser tempUser = getTempUser(signupForm, email); - - - // Send email to the user. - ActionURL confirmationUrl = getConfirmationURL(getContainer(), email, tempUser.getKey()); - try - { - User mockUser = new User(); - mockUser.setEmail(email.getEmailAddress()); - SecurityManager.sendEmail(getContainer(), mockUser, SecurityManager.getRegistrationMessage(null, false), email.getEmailAddress(), confirmationUrl); - } - catch(Exception e) - { - String systemEmail = LookAndFeelProperties.getInstance(getContainer()).getSystemEmailAddress(); - List messages = new ArrayList<>(); - messages.add("Could not send new user registration email. Please contact your server administrator at " + systemEmail); - messages.add(e.getMessage()); - response.put("error_message", messages); - } - - signupForm.setNewSignUp(false); // TODO: Most likely not needed here response.put("status", "USER_ADDED"); - return response; } } diff --git a/signup/src/org/labkey/signup/signupPage.jsp b/signup/src/org/labkey/signup/signupPage.jsp index 621c912f..269f945f 100644 --- a/signup/src/org/labkey/signup/signupPage.jsp +++ b/signup/src/org/labkey/signup/signupPage.jsp @@ -1,38 +1,37 @@ -<%@ page import="org.labkey.api.view.ActionURL" %> <%@ page import="org.labkey.api.view.HttpView" %> -<%@ page import="org.labkey.signup.SignUpController.BeginAction" %> <%@ page import="org.labkey.signup.SignUpController.SignupForm" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% SignupForm form = (SignupForm)HttpView.currentModel(); - ActionURL url = urlFor(BeginAction.class); + String contextPath = request.getContextPath(); %> - -
- - - - - - - - - - - - - - - - - - - - - -
*
*
*
*
- + + + + + + + + <%-- Verification: standalone full-width block --%> +
Verification
+

Please enter the six characters shown below (case-insensitive).

+

Get a new image.

+ Captcha + + +
+ +
+ + + From 495d02e06e08a0e4821772e3a00033df5343a07f Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Thu, 21 May 2026 13:16:12 -0700 Subject: [PATCH 3/4] Address Claude Code reviewer comments - Catch ConfigurationException from sendEmail (was only catching MessagingException) - Add status="ERROR" to API captcha-failure response for consistency with other response paths - Remove hardcoded "six characters" from kaptcha instruction text - Add aria-label to kaptcha input field - Use method reference in errorsToMessages stream Co-Authored-By: Claude --- signup/src/org/labkey/signup/SignUpController.java | 7 +++++-- signup/src/org/labkey/signup/signupPage.jsp | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 5555280f..67af0820 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -55,6 +55,7 @@ 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.PageFlowUtil; import org.labkey.api.util.URLHelper; @@ -68,6 +69,7 @@ import org.labkey.api.view.WebPartView; import org.springframework.validation.BindException; import org.springframework.validation.Errors; +import org.springframework.validation.ObjectError; import org.springframework.web.servlet.ModelAndView; import java.util.ArrayList; @@ -699,7 +701,7 @@ private boolean createUserAndSendEmail(SignupForm form, ValidEmail email, Errors SecurityManager.getRegistrationMessage(null, false), email.getEmailAddress(), confirmationUrl); } - catch (MessagingException e) + catch (MessagingException | ConfigurationException e) { String systemEmail = LookAndFeelProperties.getInstance(getContainer()).getSystemEmailAddress(); errors.reject(ERROR_MSG, "Could not send new user registration email. Please contact your server administrator at " + systemEmail); @@ -717,7 +719,7 @@ private boolean createUserAndSendEmail(SignupForm form, ValidEmail email, Errors private static List errorsToMessages(Errors errors) { return errors.getAllErrors().stream() - .map(e -> e.getDefaultMessage()) + .map(ObjectError::getDefaultMessage) .toList(); } @@ -882,6 +884,7 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E String kaptchaError = verifyCaptcha(signupForm.getKaptchaText(), signupForm.getEmail()); if (kaptchaError != null) { + response.put("status", "ERROR"); response.put("error_message", List.of(kaptchaError)); return response; } diff --git a/signup/src/org/labkey/signup/signupPage.jsp b/signup/src/org/labkey/signup/signupPage.jsp index 269f945f..91a71561 100644 --- a/signup/src/org/labkey/signup/signupPage.jsp +++ b/signup/src/org/labkey/signup/signupPage.jsp @@ -18,10 +18,10 @@ <%-- Verification: standalone full-width block --%>
Verification
-

Please enter the six characters shown below (case-insensitive).

+

Please enter the characters shown below (case-insensitive).

Get a new image.

Captcha - +
From 3e1101fd50be7ff85bc033a0559f5c78476fbaa0 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Thu, 21 May 2026 14:20:19 -0700 Subject: [PATCH 4/4] SignUpController.java: fix Copilot review comments (PR #638, round 1) * SignUpApiAction.execute: add status="ERROR" to blank-field, bad-email, and send-failure error paths (was inconsistent with captcha path) * verifyCaptcha: use parameterized logging instead of string concatenation * verifyCaptcha: fix typo in comment ("a a" -> "a") Co-Authored-By: Claude Sonnet 4.6 --- signup/src/org/labkey/signup/SignUpController.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 67af0820..9f216c0a 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -642,7 +642,7 @@ else if(form.getEmail() != null && !form.getEmail().equals(form.getEmailConfirm( } // On success returns null and clears the session attribute so the captcha cannot be replayed. - // On failure return a a user-facing error message. + // On failure return a user-facing error message. // Logging matches LoginController's RegisterUserAction. private String verifyCaptcha(String submittedText, String emailForLogging) { @@ -655,7 +655,7 @@ private String verifyCaptcha(String submittedText, String emailForLogging) } if (!expected.equalsIgnoreCase(StringUtils.trimToNull(submittedText))) { - _log.warn("Captcha text did not match for signup attempt for " + emailForLogging); + _log.warn("Captcha text did not match for signup attempt for {}", emailForLogging); return "Verification text does not match, please retry."; } session.removeAttribute(LabKeyKaptchaServlet.SESSION_KEY_VALUE); @@ -892,6 +892,7 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E validateSignupForm(signupForm, errors); if (errors.hasErrors()) { + response.put("status", "ERROR"); response.put("error_message", errorsToMessages(errors)); return response; } @@ -899,6 +900,7 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E ValidEmail email = parseAndValidateEmail(signupForm, errors); if (email == null) { + response.put("status", "ERROR"); response.put("error_message", errorsToMessages(errors)); return response; } @@ -911,6 +913,7 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E if (!createUserAndSendEmail(signupForm, email, errors)) { + response.put("status", "ERROR"); response.put("error_message", errorsToMessages(errors)); return response; }