From 049958d3f84fb52b440084e190876c5b87949a11 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Mon, 26 Oct 2026 10:51:21 -0700 Subject: [PATCH 1/3] Added hashing, expiration and single use to SignUp confirmation keys * Stored only a SHA-256 hash of the confirmation key in signup.temporaryuser, with a keyTimeout seven days out * Changed a repeat signup to replace the key and the stored name, organization and folder * Cleared the key when the account is created, so each link works once * Added the link expiration to the confirmation email * Masked the key in the admin ConfirmationURL column, and left the column empty for used keys * Added signup-26.000-26.001.sql, which hashes pending keys, sets them to expire seven days after upgrade, and clears used keys * Added SignUpManager unit tests Ports the token hashing and expiration from LabKey/platform#8211. Co-Authored-By: Claude --- .../postgresql/signup-26.000-26.001.sql | 19 +++ signup/resources/schemas/signup.xml | 1 + .../org/labkey/signup/SignUpController.java | 30 +--- .../src/org/labkey/signup/SignUpManager.java | 157 +++++++++++++++++- .../src/org/labkey/signup/SignUpModule.java | 8 +- .../src/org/labkey/signup/SignUpSchema.java | 8 +- signup/src/org/labkey/signup/TempUser.java | 15 +- 7 files changed, 207 insertions(+), 31 deletions(-) create mode 100644 signup/resources/schemas/dbscripts/postgresql/signup-26.000-26.001.sql diff --git a/signup/resources/schemas/dbscripts/postgresql/signup-26.000-26.001.sql b/signup/resources/schemas/dbscripts/postgresql/signup-26.000-26.001.sql new file mode 100644 index 00000000..a6610971 --- /dev/null +++ b/signup/resources/schemas/dbscripts/postgresql/signup-26.000-26.001.sql @@ -0,0 +1,19 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0: http://www.apache.org/licenses/LICENSE-2.0 + */ + +-- Room for the sha256: prefix plus a 64-character hash. NULL means the key was used to create an account. +ALTER TABLE signup.temporaryuser ALTER COLUMN key TYPE VARCHAR(100); +ALTER TABLE signup.temporaryuser ALTER COLUMN key DROP NOT NULL; +ALTER TABLE signup.temporaryuser ADD keyTimeout TIMESTAMP; + +-- Rows with a LabKey userId already used their key to create the account. +UPDATE signup.temporaryuser SET key = NULL WHERE labkeyUserId IS NOT NULL; + +-- Must match SignUpManager.hashKey(). Pending links expire seven days after upgrade. +UPDATE signup.temporaryuser +SET key = 'sha256:' || encode(sha256(convert_to(key, 'UTF8')), 'hex'), + keyTimeout = LOCALTIMESTAMP + INTERVAL '7 days' +WHERE key IS NOT NULL; diff --git a/signup/resources/schemas/signup.xml b/signup/resources/schemas/signup.xml index 35e7bd26..943f0c15 100644 --- a/signup/resources/schemas/signup.xml +++ b/signup/resources/schemas/signup.xml @@ -27,6 +27,7 @@ + diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index a51533fd..1767df54 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -466,8 +466,7 @@ public boolean handlePost(SignupConfirmForm form, BindException errors) throws E if (result != null) _successUrl = result.getRedirectURL(); - _tempUser.setLabkeyUserId(newUser.getUserId()); - Table.update(null, SignUpManager.getTableInfoTempUsers(), _tempUser, _tempUser.getUserId()); + SignUpManager.get().markKeyUsed(_tempUser, newUser.getUserId()); Container c = _tempUser.getContainer(); WritablePropertyMap property = PropertyManager.getWritableProperties(c, SignUpModule.SIGNUP_CATEGORY, false); @@ -729,13 +728,14 @@ private void createUserAndSendEmail(SignupForm form, ValidEmail email) { try (DbScope.Transaction transaction = SignUpManager.getSchema().getScope().ensureTransaction()) { - TempUser tempUser = getTempUser(form, email); - ActionURL confirmationUrl = getConfirmationURL(getContainer(), email, tempUser.getKey()); + String key = SignUpManager.get().saveWithNewKey(email, form.getFirstName(), form.getLastName(), + form.getOrganization(), getContainer()); + ActionURL confirmationUrl = getConfirmationURL(getContainer(), email, key); User mockUser = new User(); mockUser.setEmail(email.getEmailAddress()); SecurityManager.sendEmail(getContainer(), mockUser, SecurityManager.getRegistrationMessage(null, false), - email.getEmailAddress(), confirmationUrl); + email.getEmailAddress(), confirmationUrl, LoginManager.SETUP_LINK_TIMEOUT); transaction.commit(); } } @@ -1029,24 +1029,4 @@ public ApiResponse execute(SignupForm signupForm, BindException errors) throws E return response; } } - - private TempUser getTempUser(SignupForm signupForm, ValidEmail email) - { - // If the user does not exit in LabKey's core database, check in our temporaryusers table - TempUser tempUser = SignUpManager.get().getTempUserWithEmail(email.getEmailAddress()); - - if(tempUser == null) - { // If tempUser is null create a new TempUser object and populate fields - tempUser = new TempUser(); - tempUser.setEmail(email.getEmailAddress()); - tempUser.setFirstName(signupForm.getFirstName()); - tempUser.setLastName(signupForm.getLastName()); - tempUser.setOrganization(signupForm.getOrganization()); - tempUser.setKey(LoginManager.createTempPassword()); - tempUser.setContainer(getContainer()); - - Table.insert(null, SignUpManager.getTableInfoTempUsers(), tempUser); - } - return tempUser; - } } \ No newline at end of file diff --git a/signup/src/org/labkey/signup/SignUpManager.java b/signup/src/org/labkey/signup/SignUpManager.java index c4655364..b0af2421 100644 --- a/signup/src/org/labkey/signup/SignUpManager.java +++ b/signup/src/org/labkey/signup/SignUpManager.java @@ -16,19 +16,33 @@ package org.labkey.signup; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.labkey.api.data.CompareType; +import org.labkey.api.data.Container; import org.labkey.api.data.DbSchema; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; import org.labkey.api.query.FieldKey; +import org.labkey.api.security.Crypt; +import org.labkey.api.security.LoginManager; import org.labkey.api.security.ValidEmail; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; + +import java.time.Instant; +import java.util.Date; public class SignUpManager { // Returned messages public static final String CONFIRMATION_DID_NOT_MATCH = "The email address and confirmation key could not be verified. " + - "Please make sure that you copied the entire link into the browser's address bar."; + "The link may have expired or already been used. Please make sure that you copied the entire link " + + "into the browser's address bar, or sign up again to receive a new link."; // TODO: Add a link to reset password in this message. public static final String USER_ALREADY_EXISTS = "A user with the email %s already exists. " + "If you have forgotten your password, you can click the \"Forgot your password?\" " + @@ -65,16 +79,155 @@ public TempUser getTempUserWithEmail(String email) } + // Returns the pending user whose unexpired, unused key matches public TempUser verifyUser(ValidEmail email, String key) { + if (key == null) + return null; SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("email"), email.getEmailAddress()); - filter.addCondition(FieldKey.fromParts("key"), key); + filter.addCondition(FieldKey.fromParts("key"), hashKey(key)); + filter.addCondition(FieldKey.fromParts("keyTimeout"), new Date(), CompareType.GT); return new TableSelector(getTableInfoTempUsers(),filter, null).getObject(TempUser.class); } + // Saves a new key for the pending user, and returns the key. A repeat signup for the same email replaces + // the key, so the link already sent stops working. The name, organization and folder from the latest + // submitted form replace the stored ones. + public String saveWithNewKey(ValidEmail email, String firstName, String lastName, String organization, Container container) + { + TempUser tempUser = getTempUserWithEmail(email.getEmailAddress()); + boolean exists = tempUser != null; + if (!exists) + { + tempUser = new TempUser(); + tempUser.setEmail(email.getEmailAddress()); + } + tempUser.setFirstName(firstName); + tempUser.setLastName(lastName); + tempUser.setOrganization(organization); + tempUser.setContainer(container); + + String key = assignKey(tempUser); + if (exists) + Table.update(null, getTableInfoTempUsers(), tempUser, tempUser.getUserId()); + else + Table.insert(null, getTableInfoTempUsers(), tempUser); + return key; + } + + // Store the labkey UserId created for the user, and clear the key so the link cannot be used again. + public void markKeyUsed(TempUser tempUser, int labkeyUserId) + { + tempUser.setLabkeyUserId(labkeyUserId); + tempUser.setKey(null); + tempUser.setKeyTimeout(null); + Table.update(null, getTableInfoTempUsers(), tempUser, tempUser.getUserId()); + } + + // Sets a new key on tempUser, storing only its hash, and returns the key. The caller saves tempUser. + private String assignKey(TempUser tempUser) + { + String key = LoginManager.createTempPassword(); + tempUser.setKey(hashKey(key)); + tempUser.setKeyTimeout(Date.from(Instant.now().plus(LoginManager.SETUP_LINK_TIMEOUT))); + return key; + } + + // Keys are long and random, so a fast unsalted hash suffices. signup-26.000-26.001.sql hashes existing rows identically. + private static String hashKey(String key) + { + return Crypt.SHA256.digestWithPrefix(key); + } + public void deleteUser(String email) { if (null != getTableInfoTempUsers()) Table.delete(getTableInfoTempUsers(), new SimpleFilter(FieldKey.fromParts("email"), email)); } + + public static class TestCase extends Assert + { + private static final String EMAIL = "signup-key-test@signup.test"; + + private ValidEmail _email; + private TempUser _tempUser; + private String _key; + + @Before + public void setUp() throws ValidEmail.InvalidEmailException + { + _email = new ValidEmail(EMAIL); + get().deleteUser(EMAIL); + + _key = get().saveWithNewKey(_email, "Signup", "Test", "Signup Test", JunitUtil.getTestContainer()); + _tempUser = get().getTempUserWithEmail(EMAIL); + } + + @After + public void tearDown() + { + get().deleteUser(EMAIL); + } + + private void save() + { + Table.update(null, getTableInfoTempUsers(), _tempUser, _tempUser.getUserId()); + } + + @Test + public void testKeyStoredAsHash() + { + TempUser stored = get().getTempUserWithEmail(EMAIL); + assertNotEquals("Key stored in plain text", _key, stored.getKey()); + assertTrue("Stored key is not a SHA-256 hash", stored.getKey().startsWith("sha256:")); + assertNotNull("Key rejected", get().verifyUser(_email, _key)); + + long expected = Instant.now().plus(LoginManager.SETUP_LINK_TIMEOUT).toEpochMilli(); + assertTrue("Key does not expire SETUP_LINK_TIMEOUT after signup", + Math.abs(stored.getKeyTimeout().getTime() - expected) < 60_000); + } + + @Test + public void testWrongKeyRejected() + { + assertNull("Wrong key accepted", get().verifyUser(_email, _key + "x")); + assertNull("Missing key accepted", get().verifyUser(_email, null)); + assertNull("Stored hash accepted as a key", get().verifyUser(_email, _tempUser.getKey())); + } + + @Test + public void testExpiredKeyRejected() + { + _tempUser.setKeyTimeout(Date.from(Instant.now().minusSeconds(1))); + save(); + assertNull("Expired key accepted", get().verifyUser(_email, _key)); + } + + @Test + public void testRepeatSignupReplacesKeyAndDetails() + { + String newKey = get().saveWithNewKey(_email, "Corrected", "Name", "New Org", JunitUtil.getTestContainer()); + assertNull("Replaced key accepted", get().verifyUser(_email, _key)); + + TempUser stored = get().verifyUser(_email, newKey); + assertNotNull("New key rejected", stored); + assertEquals("Repeat signup created a second row", _tempUser.getUserId(), stored.getUserId()); + assertEquals("First name not updated", "Corrected", stored.getFirstName()); + assertEquals("Last name not updated", "Name", stored.getLastName()); + assertEquals("Organization not updated", "New Org", stored.getOrganization()); + } + + @Test + public void testUsedKeyRejected() + { + int userId = TestContext.get().getUser().getUserId(); + get().markKeyUsed(_tempUser, userId); + assertNull("Used key accepted", get().verifyUser(_email, _key)); + + TempUser stored = get().getTempUserWithEmail(EMAIL); + assertNull("Key not cleared after use", stored.getKey()); + assertNull("Key timeout not cleared after use", stored.getKeyTimeout()); + assertEquals("Account not recorded on the signup row", Integer.valueOf(userId), stored.getLabkeyUserId()); + } + } } \ No newline at end of file diff --git a/signup/src/org/labkey/signup/SignUpModule.java b/signup/src/org/labkey/signup/SignUpModule.java index c7205424..bd4059e4 100644 --- a/signup/src/org/labkey/signup/SignUpModule.java +++ b/signup/src/org/labkey/signup/SignUpModule.java @@ -52,7 +52,7 @@ public String getName() @Override public @Nullable Double getSchemaVersion() { - return 13.24; + return 26.001; } @Override @@ -113,4 +113,10 @@ public boolean getRequireSitePermission() return true; } + @Override + public @NotNull Set> getUnitTests() + { + return Set.of(SignUpManager.TestCase.class); + } + } \ No newline at end of file diff --git a/signup/src/org/labkey/signup/SignUpSchema.java b/signup/src/org/labkey/signup/SignUpSchema.java index 25026e1b..a1ddc9c8 100644 --- a/signup/src/org/labkey/signup/SignUpSchema.java +++ b/signup/src/org/labkey/signup/SignUpSchema.java @@ -121,13 +121,17 @@ public DisplayColumn createRenderer(ColumnInfo colInfo) @Override public void renderGridCellContents(RenderContext ctx, HtmlWriter out) { + // The key is cleared once the link is used + if (getValue(ctx) == null) + return; + Container container = ContainerManager.getForId(String.valueOf(ctx.get(FieldKey.fromParts("Container")))); String email = String.valueOf(ctx.get(FieldKey.fromParts("email"))); - String key = String.valueOf(ctx.get(getColumnInfo().getFieldKey())); + // Only a hash of the key is stored, so the link is shown masked ActionURL url = new ActionURL(SignUpController.ConfirmAction.class, container); url.addParameter("email", email); - url.addParameter("key", key); + url.addParameter("key", "**********"); out.write(url.getLocalURIString()); // out.write("Confirmation URL"); } diff --git a/signup/src/org/labkey/signup/TempUser.java b/signup/src/org/labkey/signup/TempUser.java index f8cedfce..67c3e4d4 100644 --- a/signup/src/org/labkey/signup/TempUser.java +++ b/signup/src/org/labkey/signup/TempUser.java @@ -16,11 +16,14 @@ package org.labkey.signup; import org.labkey.api.data.Container; +import java.util.Date; + // Class TempUser contains information needed to add a user to the LabKey database as a User later on but has addition -// field @key for use with confirmation +// field @key for use with confirmation. The key holds a hash of the emailed key, or null once the key has been used. public class TempUser { private String key; + private Date keyTimeout; private String email; private String firstName; private String lastName; @@ -100,6 +103,16 @@ public void setKey(String key) this.key = key; } + public Date getKeyTimeout() + { + return keyTimeout; + } + + public void setKeyTimeout(Date keyTimeout) + { + this.keyTimeout = keyTimeout; + } + public Integer getLabkeyUserId() { return _labkeyUserId; From c0f06c793cafe35870c9af6ce15023dd00e4bc44 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Mon, 26 Oct 2026 14:23:46 -0700 Subject: [PATCH 2/3] Fixed SignUp key handling found in review * Added SignUpManager.clearKeyIfValid, which clears the key only if it still matches and has not expired, and changed ConfirmAction to call it first in the account transaction and roll back when it returns false * Changed a repeat signup to replace only the key, keeping the first signup's name, organization and folder * Replaced the full-row saves in saveWithNewKey and setLabkeyUserId with Table.update calls that set only the changed columns * Changed the ConfirmationURL column to show the masked link only for unused, unexpired keys, in the grid, exports and the JSON API * Moved SignUpManager.TestCase to getIntegrationTests, since it needs the database * Added tests for replaced, used and expired keys Co-Authored-By: Claude --- .../org/labkey/signup/SignUpController.java | 13 ++- .../src/org/labkey/signup/SignUpManager.java | 85 ++++++++++++------- .../src/org/labkey/signup/SignUpModule.java | 2 +- .../src/org/labkey/signup/SignUpSchema.java | 34 ++++++-- 4 files changed, 89 insertions(+), 45 deletions(-) diff --git a/signup/src/org/labkey/signup/SignUpController.java b/signup/src/org/labkey/signup/SignUpController.java index 1767df54..54d0c64b 100644 --- a/signup/src/org/labkey/signup/SignUpController.java +++ b/signup/src/org/labkey/signup/SignUpController.java @@ -450,6 +450,11 @@ public boolean handlePost(SignupConfirmForm form, BindException errors) throws E // User creation should be in a transaction try (DbScope.Transaction transaction = CoreSchema.getInstance().getSchema().getScope().ensureTransaction()) { + // Clear the key so it cannot be reused. Returns false if the key was replaced, cleared or expired since + // validateRequest. The reshown view reports the error. + if (!SignUpManager.get().clearKeyIfValid(_tempUser)) + return false; + // Add user to LabKey core database SecurityManager.NewUserStatus newUserStatus = SecurityManager.addUser(_email, null); User newUser = newUserStatus.getUser(); @@ -466,7 +471,7 @@ public boolean handlePost(SignupConfirmForm form, BindException errors) throws E if (result != null) _successUrl = result.getRedirectURL(); - SignUpManager.get().markKeyUsed(_tempUser, newUser.getUserId()); + SignUpManager.get().setLabkeyUserId(_tempUser, newUser.getUserId()); Container c = _tempUser.getContainer(); WritablePropertyMap property = PropertyManager.getWritableProperties(c, SignUpModule.SIGNUP_CATEGORY, false); @@ -720,9 +725,9 @@ private ValidEmail parseAndValidateEmail(SignupForm form, Errors errors) } } - // Creates (or reuses) a TempUser row and sends the confirmation email in a single - // transaction. On send failure the exception propagates and the transaction rolls back - // automatically so a freshly inserted TempUser row does not persist. + // Saves the TempUser row with a new key and sends the confirmation email in one transaction. A repeat + // signup replaces the key, so the link already sent stops working. If the email cannot be sent, nothing + // is saved, and the earlier link keeps working. private void createUserAndSendEmail(SignupForm form, ValidEmail email) throws MessagingException, ConfigurationException, java.sql.SQLException { diff --git a/signup/src/org/labkey/signup/SignUpManager.java b/signup/src/org/labkey/signup/SignUpManager.java index b0af2421..10c1dc07 100644 --- a/signup/src/org/labkey/signup/SignUpManager.java +++ b/signup/src/org/labkey/signup/SignUpManager.java @@ -23,7 +23,9 @@ import org.labkey.api.data.CompareType; import org.labkey.api.data.Container; import org.labkey.api.data.DbSchema; +import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; +import org.labkey.api.data.SqlExecutor; import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; @@ -36,6 +38,8 @@ import java.time.Instant; import java.util.Date; +import java.util.HashMap; +import java.util.Map; public class SignUpManager { @@ -90,38 +94,48 @@ public TempUser verifyUser(ValidEmail email, String key) return new TableSelector(getTableInfoTempUsers(),filter, null).getObject(TempUser.class); } - // Saves a new key for the pending user, and returns the key. A repeat signup for the same email replaces - // the key, so the link already sent stops working. The name, organization and folder from the latest - // submitted form replace the stored ones. + // Saves and returns a new key for the pending user. A repeat signup for the same email replaces the key, + // so the link sent with the old key stops working. public String saveWithNewKey(ValidEmail email, String firstName, String lastName, String organization, Container container) { TempUser tempUser = getTempUserWithEmail(email.getEmailAddress()); - boolean exists = tempUser != null; - if (!exists) + if (tempUser != null) { - tempUser = new TempUser(); - tempUser.setEmail(email.getEmailAddress()); + String key = assignKey(tempUser); + Map keyFields = new HashMap<>(); + keyFields.put("key", tempUser.getKey()); + keyFields.put("keyTimeout", tempUser.getKeyTimeout()); + Table.update(null, getTableInfoTempUsers(), keyFields, tempUser.getUserId()); + return key; } + + tempUser = new TempUser(); + tempUser.setEmail(email.getEmailAddress()); tempUser.setFirstName(firstName); tempUser.setLastName(lastName); tempUser.setOrganization(organization); tempUser.setContainer(container); - String key = assignKey(tempUser); - if (exists) - Table.update(null, getTableInfoTempUsers(), tempUser, tempUser.getUserId()); - else - Table.insert(null, getTableInfoTempUsers(), tempUser); + Table.insert(null, getTableInfoTempUsers(), tempUser); return key; } - // Store the labkey UserId created for the user, and clear the key so the link cannot be used again. - public void markKeyUsed(TempUser tempUser, int labkeyUserId) + // Clears the key so the link cannot be used again. Returns false if the key has been replaced, used or has + // expired since tempUser was read. A concurrent call for the same row waits until this transaction ends. + public boolean clearKeyIfValid(TempUser tempUser) + { + SQLFragment sql = SQLFragment.unsafe("UPDATE ").append(getTableInfoTempUsers()) + .unsafeAppend(" SET Key = NULL, KeyTimeout = NULL WHERE UserId = ? AND Key = ? AND KeyTimeout > ?", + tempUser.getUserId(), tempUser.getKey(), new Date()); + return new SqlExecutor(getSchema()).execute(sql) == 1; + } + + // Stores the id of the LabKey account created from tempUser + public void setLabkeyUserId(TempUser tempUser, int labkeyUserId) { - tempUser.setLabkeyUserId(labkeyUserId); - tempUser.setKey(null); - tempUser.setKeyTimeout(null); - Table.update(null, getTableInfoTempUsers(), tempUser, tempUser.getUserId()); + Map fields = new HashMap<>(); + fields.put("labkeyUserId", labkeyUserId); + Table.update(null, getTableInfoTempUsers(), fields, tempUser.getUserId()); } // Sets a new key on tempUser, storing only its hash, and returns the key. The caller saves tempUser. @@ -133,7 +147,8 @@ private String assignKey(TempUser tempUser) return key; } - // Keys are long and random, so a fast unsalted hash suffices. signup-26.000-26.001.sql hashes existing rows identically. + // Keys are long and random, so a fast unsalted hash suffices. signup-26.000-26.001.sql hashes existing rows + // identically. private static String hashKey(String key) { return Crypt.SHA256.digestWithPrefix(key); @@ -178,13 +193,8 @@ private void save() public void testKeyStoredAsHash() { TempUser stored = get().getTempUserWithEmail(EMAIL); - assertNotEquals("Key stored in plain text", _key, stored.getKey()); - assertTrue("Stored key is not a SHA-256 hash", stored.getKey().startsWith("sha256:")); + assertEquals("Key not stored as its SHA-256 hash", Crypt.SHA256.digestWithPrefix(_key), stored.getKey()); assertNotNull("Key rejected", get().verifyUser(_email, _key)); - - long expected = Instant.now().plus(LoginManager.SETUP_LINK_TIMEOUT).toEpochMilli(); - assertTrue("Key does not expire SETUP_LINK_TIMEOUT after signup", - Math.abs(stored.getKeyTimeout().getTime() - expected) < 60_000); } @Test @@ -201,33 +211,44 @@ public void testExpiredKeyRejected() _tempUser.setKeyTimeout(Date.from(Instant.now().minusSeconds(1))); save(); assertNull("Expired key accepted", get().verifyUser(_email, _key)); + assertFalse("Expired key cleared", get().clearKeyIfValid(_tempUser)); } @Test - public void testRepeatSignupReplacesKeyAndDetails() + public void testRepeatSignupReplacesOnlyKey() { - String newKey = get().saveWithNewKey(_email, "Corrected", "Name", "New Org", JunitUtil.getTestContainer()); + String newKey = get().saveWithNewKey(_email, "Other", "Name", "Other Org", JunitUtil.getTestContainer()); assertNull("Replaced key accepted", get().verifyUser(_email, _key)); TempUser stored = get().verifyUser(_email, newKey); assertNotNull("New key rejected", stored); assertEquals("Repeat signup created a second row", _tempUser.getUserId(), stored.getUserId()); - assertEquals("First name not updated", "Corrected", stored.getFirstName()); - assertEquals("Last name not updated", "Name", stored.getLastName()); - assertEquals("Organization not updated", "New Org", stored.getOrganization()); + assertEquals("First name changed by a repeat signup", "Signup", stored.getFirstName()); + assertEquals("Last name changed by a repeat signup", "Test", stored.getLastName()); + assertEquals("Organization changed by a repeat signup", "Signup Test", stored.getOrganization()); + } + + @Test + public void testReplacedKeyCannotBeUsed() + { + String newKey = get().saveWithNewKey(_email, "Signup", "Test", "Signup Test", JunitUtil.getTestContainer()); + assertFalse("Replaced key cleared", get().clearKeyIfValid(_tempUser)); + assertNotNull("New key cleared by the replaced key", get().verifyUser(_email, newKey)); } @Test public void testUsedKeyRejected() { int userId = TestContext.get().getUser().getUserId(); - get().markKeyUsed(_tempUser, userId); + assertTrue("Valid key not cleared", get().clearKeyIfValid(_tempUser)); + get().setLabkeyUserId(_tempUser, userId); assertNull("Used key accepted", get().verifyUser(_email, _key)); + assertFalse("Key cleared twice", get().clearKeyIfValid(_tempUser)); TempUser stored = get().getTempUserWithEmail(EMAIL); assertNull("Key not cleared after use", stored.getKey()); assertNull("Key timeout not cleared after use", stored.getKeyTimeout()); - assertEquals("Account not recorded on the signup row", Integer.valueOf(userId), stored.getLabkeyUserId()); + assertEquals("Account userId not saved on the signup row", Integer.valueOf(userId), stored.getLabkeyUserId()); } } } \ No newline at end of file diff --git a/signup/src/org/labkey/signup/SignUpModule.java b/signup/src/org/labkey/signup/SignUpModule.java index bd4059e4..035c6f8d 100644 --- a/signup/src/org/labkey/signup/SignUpModule.java +++ b/signup/src/org/labkey/signup/SignUpModule.java @@ -114,7 +114,7 @@ public boolean getRequireSitePermission() } @Override - public @NotNull Set> getUnitTests() + public @NotNull Set> getIntegrationTests() { return Set.of(SignUpManager.TestCase.class); } diff --git a/signup/src/org/labkey/signup/SignUpSchema.java b/signup/src/org/labkey/signup/SignUpSchema.java index a1ddc9c8..60a58104 100644 --- a/signup/src/org/labkey/signup/SignUpSchema.java +++ b/signup/src/org/labkey/signup/SignUpSchema.java @@ -42,6 +42,7 @@ import org.labkey.api.view.ActionURL; import org.labkey.api.writer.HtmlWriter; +import java.util.Date; import java.util.Set; public class SignUpSchema extends UserSchema @@ -118,21 +119,37 @@ public DisplayColumn createRenderer(ColumnInfo colInfo) { return new DataColumn(colInfo) { + // Returns the link with the key masked while the key is unused and unexpired, + // otherwise null. Only a hash of the key is stored. Exports use this value, so the + // hash is not exported. @Override - public void renderGridCellContents(RenderContext ctx, HtmlWriter out) + public Object getDisplayValue(RenderContext ctx) { - // The key is cleared once the link is used - if (getValue(ctx) == null) - return; + Date keyTimeout = ctx.get(FieldKey.fromParts("keyTimeout"), Date.class); + if (getValue(ctx) == null || keyTimeout == null || !keyTimeout.after(new Date())) + return null; - Container container = ContainerManager.getForId(String.valueOf(ctx.get(FieldKey.fromParts("Container")))); - String email = String.valueOf(ctx.get(FieldKey.fromParts("email"))); + Container container = ContainerManager.getForId(ctx.get(FieldKey.fromParts("Container"), String.class)); + String email = ctx.get(FieldKey.fromParts("email"), String.class); - // Only a hash of the key is stored, so the link is shown masked ActionURL url = new ActionURL(SignUpController.ConfirmAction.class, container); url.addParameter("email", email); url.addParameter("key", "**********"); - out.write(url.getLocalURIString()); + return url.getLocalURIString(); + } + + @Override + public Object getJsonValue(RenderContext ctx) + { + return getDisplayValue(ctx); + } + + @Override + public void renderGridCellContents(RenderContext ctx, HtmlWriter out) + { + Object url = getDisplayValue(ctx); + if (url != null) + out.write(url.toString()); // out.write("Confirmation URL"); } @@ -142,6 +159,7 @@ public void addQueryFieldKeys(Set keys) super.addQueryFieldKeys(keys); keys.add(FieldKey.fromParts("Container")); keys.add(FieldKey.fromParts("email")); + keys.add(FieldKey.fromParts("keyTimeout")); } }; } From 958be17f5cf7bb93445f791e083ca1a936cfdbbf Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Mon, 26 Oct 2026 19:32:52 -0700 Subject: [PATCH 3/3] Annotated SignUpManager.verifyUser with @Nullable * Marked the return value and the key parameter @Nullable Co-Authored-By: Claude --- signup/src/org/labkey/signup/SignUpManager.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/signup/src/org/labkey/signup/SignUpManager.java b/signup/src/org/labkey/signup/SignUpManager.java index 10c1dc07..b2d816ba 100644 --- a/signup/src/org/labkey/signup/SignUpManager.java +++ b/signup/src/org/labkey/signup/SignUpManager.java @@ -16,6 +16,7 @@ package org.labkey.signup; +import org.jetbrains.annotations.Nullable; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -84,7 +85,7 @@ public TempUser getTempUserWithEmail(String email) } // Returns the pending user whose unexpired, unused key matches - public TempUser verifyUser(ValidEmail email, String key) + public @Nullable TempUser verifyUser(ValidEmail email, @Nullable String key) { if (key == null) return null;