From 223664bc3d1e154c5745dfc3b67295d1d6366952 Mon Sep 17 00:00:00 2001 From: labkey-sweta Date: Wed, 15 Jan 2025 14:23:33 -0800 Subject: [PATCH 1/5] Automation test for invalidate sessions. --- .../pages/core/admin/CustomizeSitePage.java | 1 + src/org/labkey/test/tests/ApiKeyTest.java | 101 ++++++++--- .../test/tests/InvalidateSessionTest.java | 164 ++++++++++++++++++ 3 files changed, 244 insertions(+), 22 deletions(-) create mode 100644 src/org/labkey/test/tests/InvalidateSessionTest.java diff --git a/src/org/labkey/test/pages/core/admin/CustomizeSitePage.java b/src/org/labkey/test/pages/core/admin/CustomizeSitePage.java index 6bafa8dcef..fa03ecc82f 100644 --- a/src/org/labkey/test/pages/core/admin/CustomizeSitePage.java +++ b/src/org/labkey/test/pages/core/admin/CustomizeSitePage.java @@ -295,6 +295,7 @@ public enum XFrameOption public enum KeyExpirationOptions implements OptionSelect.SelectOption { UNLIMITED(-1), + TEN_SECONDS(10), ONE_WEEK(7*SECONDS_PER_DAY), ONE_MONTH(30*SECONDS_PER_DAY), THREE_MONTHS(90*SECONDS_PER_DAY), diff --git a/src/org/labkey/test/tests/ApiKeyTest.java b/src/org/labkey/test/tests/ApiKeyTest.java index 6d1873598c..05bfc89380 100644 --- a/src/org/labkey/test/tests/ApiKeyTest.java +++ b/src/org/labkey/test/tests/ApiKeyTest.java @@ -72,13 +72,6 @@ public class ApiKeyTest extends BaseWebDriverTest private static final String API_USERNAME = "apikey"; private static final TestUser EDITOR_USER = new TestUser("editor@apikey.test"); - @Override - protected void doCleanup(boolean afterTest) throws TestTimeoutException - { - super.doCleanup(afterTest); - _userHelper.deleteUsers(false, EDITOR_USER); - } - @BeforeClass public static void setupProject() { @@ -87,6 +80,13 @@ public static void setupProject() init.doSetup(); } + @Override + protected void doCleanup(boolean afterTest) throws TestTimeoutException + { + super.doCleanup(afterTest); + _userHelper.deleteUsers(false, EDITOR_USER); + } + private void doSetup() { _containerHelper.createProject(getProjectName(), null); @@ -118,20 +118,25 @@ public void testSessionKey() throws IOException signOut(); log("Verify that logging out invalidates session keys"); - verifyInvalidAPIKey(apiKey); + verifyInvalidAPIKey(apiKey, null); simpleSignIn(); log("Verify that session keys remain invalid after logging back in"); - verifyInvalidAPIKey(apiKey); + verifyInvalidAPIKey(apiKey, null); } private void verifyValidAPIKey(String apiKey) throws IOException { - verifyValidAPIKey(apiKey, false); + verifyValidAPIKey(apiKey, false, null); } - private void verifyValidAPIKey(String apiKey, boolean basicAuth) throws IOException + private Connection verifyValidAPIKey(String apiKey, boolean basicAuth, @Nullable Connection connection) throws IOException { - Connection cn = new Connection(WebTestHelper.getBaseURL(), basicAuth ? new BasicAuthCredentialsProvider(API_USERNAME, apiKey) : new ApiKeyCredentialsProvider(apiKey)); + Connection cn; + if (connection == null) + cn = new Connection(WebTestHelper.getBaseURL(), basicAuth ? new BasicAuthCredentialsProvider(API_USERNAME, apiKey) + : new ApiKeyCredentialsProvider(apiKey)); + else + cn = connection; try { GetSchemasCommand cmd = new GetSchemasCommand(); @@ -145,12 +150,17 @@ private void verifyValidAPIKey(String apiKey, boolean basicAuth) throws IOExcept { throw new RuntimeException("Response: " + e.getStatusCode(), e); } + return cn; } - private void verifyInvalidAPIKey(String apiKey) throws IOException + private void verifyInvalidAPIKey(String apiKey, @Nullable Connection connection) throws IOException { boolean isSessionKey = !apiKey.startsWith(API_USERNAME); - Connection cn = new Connection(WebTestHelper.getBaseURL(), new ApiKeyCredentialsProvider(apiKey)); + Connection cn; + if (connection == null) + cn = new Connection(WebTestHelper.getBaseURL(), new ApiKeyCredentialsProvider(apiKey)); + else + cn = connection; try { GetSchemasCommand cmd = new GetSchemasCommand(); @@ -160,7 +170,7 @@ private void verifyInvalidAPIKey(String apiKey) throws IOException else fail("API key should no longer be valid"); } - catch(CommandException e) + catch (CommandException e) { assertEquals("Wrong response for invalid " + (isSessionKey ? "session" : "API") + " key", HttpStatus.SC_UNAUTHORIZED, e.getStatusCode()); log("Success: command failed as expected."); @@ -188,8 +198,8 @@ public void testNonAdminUser() throws IOException int beforeDeleteCount = grid.getRecordCount(); assertFalse("Row with description not found", grid.getRowMap("Description", keyDescription).isEmpty()); grid = deleteAPIKeyViaUI(); - assertEquals("Number of keys after UI deletion not as expected", beforeDeleteCount-1, grid.getRecordCount()); - verifyInvalidAPIKey(apiKey); + assertEquals("Number of keys after UI deletion not as expected", beforeDeleteCount - 1, grid.getRecordCount()); + verifyInvalidAPIKey(apiKey, null); } @Test @@ -207,7 +217,7 @@ public void testStandardApiKey() throws IOException log("Verify active API key via api authentication"); verifyValidAPIKey(apiKey); log("Verify active API key via basic authentication"); - verifyValidAPIKey(apiKey, true); + verifyValidAPIKey(apiKey, true, null); log("Generate two other keys for use in testing deletion."); generateAPIKey(null); @@ -215,7 +225,7 @@ public void testStandardApiKey() throws IOException QueryGrid grid = new QueryGrid.QueryGridFinder(getDriver()).waitFor(); int beforeDeleteCount = grid.getRecordCount(); grid = deleteAPIKeyViaUI(); - assertEquals("Number of keys after UI deletion not as expected", beforeDeleteCount-1, grid.getRecordCount()); + assertEquals("Number of keys after UI deletion not as expected", beforeDeleteCount - 1, grid.getRecordCount()); log("Verify existing active API key with disabled api key setting"); goToAdminConsole() @@ -226,13 +236,60 @@ public void testStandardApiKey() throws IOException log("Verify key deletion via UI with disabled api key generation works."); grid = deleteAPIKeyViaUI(); - assertEquals("Number of keys after UI deletion not as expected", beforeDeleteCount-2, grid.getRecordCount()); + assertEquals("Number of keys after UI deletion not as expected", beforeDeleteCount - 2, grid.getRecordCount()); // skip testing api key expiration since it's already covered in unit test and 10 seconds expiration option is dev mode only log("Verify revoked/deleted api key"); deleteAPIKeys(_generatedApiKeys); - verifyInvalidAPIKey(apiKey); + verifyInvalidAPIKey(apiKey, null); + } + + /* + Regression coverage for Secure Issue 51637: Invalidate sessions when their API key becomes invalid + */ + @Test + public void testSessionInvalidatesAfterAPIKeyChange() throws IOException + { + List> _generatedApiKeys = new ArrayList<>(); + + log("Generating an apikey which expire in one week"); + goToAdminConsole() + .clickSiteSettings() + .setAllowApiKeys(true) + .setApiKeyExpiration(CustomizeSitePage.KeyExpirationOptions.ONE_WEEK) + .save(); + + String apiKey1 = generateAPIKeyAndRecord(_generatedApiKeys); + Connection cn = verifyValidAPIKey(apiKey1, false, null); + + log("Deleting the apikey"); + deleteAPIKeys(_generatedApiKeys); + + log("Verifying the session associated with deleted apikey is invalid"); + verifyInvalidAPIKey(apiKey1, cn); + + log("Verifying that new connection cannot be created after apikey is deleted"); + verifyInvalidAPIKey(apiKey1, null); + + log("Generating the apikey which expires in ten seconds"); + goToAdminConsole() + .clickSiteSettings() + .setAllowApiKeys(true) + .setApiKeyExpiration(CustomizeSitePage.KeyExpirationOptions.TEN_SECONDS) + .save(); + + log("Verify apikey generation fails"); + goToExternalToolPage(); + String apikey2 = ApiKeyPanel.panelFinder(getDriver()).find().generateApiKey(); + + log("Verify valid apikey cannot be created before expiring"); + verifyValidAPIKey(apikey2, false, null); + + sleep(10000); // Wait for apikey to expire + + log("Verify apikey cannot be created after the timer is expired"); + verifyInvalidAPIKey(apikey2, null); } @Test @@ -413,7 +470,7 @@ protected Map getLastAPIKeyRecord() throws IOException String keyField = "RowId"; Map record = response.getRows().get(0); Map newRow = new HashMap<>(); - Integer rowId = (Integer)((Map)record.get(keyField)).get("value"); + Integer rowId = (Integer) ((Map) record.get(keyField)).get("value"); newRow.put(keyField, rowId); return newRow; diff --git a/src/org/labkey/test/tests/InvalidateSessionTest.java b/src/org/labkey/test/tests/InvalidateSessionTest.java new file mode 100644 index 0000000000..826f723dda --- /dev/null +++ b/src/org/labkey/test/tests/InvalidateSessionTest.java @@ -0,0 +1,164 @@ +package org.labkey.test.tests; + +import org.apache.hc.core5.http.HttpStatus; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.labkey.remoteapi.CommandException; +import org.labkey.remoteapi.Connection; +import org.labkey.remoteapi.query.SelectRowsCommand; +import org.labkey.remoteapi.query.SelectRowsResponse; +import org.labkey.test.BaseWebDriverTest; +import org.labkey.test.categories.Daily; +import org.labkey.test.components.core.login.SetPasswordForm; +import org.labkey.test.util.ApiPermissionsHelper; +import org.labkey.test.util.PasswordUtil; +import org.labkey.test.util.PermissionsHelper; +import org.openqa.selenium.Cookie; + +import java.io.IOException; +import java.util.Arrays; +import java.util.List; +import java.util.Set; + +import static org.junit.Assert.fail; + +@Category({Daily.class}) +public class InvalidateSessionTest extends BaseWebDriverTest +{ + private static final String USER = "pwd_change_user@invalidatesessiontest.test"; + + @BeforeClass + public static void setupProject() + { + InvalidateSessionTest init = getCurrentTest(); + init.doSetup(); + } + + @Override + protected void doCleanup(boolean afterTest) + { + _containerHelper.deleteProject(getProjectName(), afterTest); + _userHelper.deleteUsers(afterTest, USER); + } + + private void doSetup() + { + _containerHelper.createProject(getProjectName(), null); + _userHelper.createUser(USER); + + ApiPermissionsHelper permissionsHelper = new ApiPermissionsHelper(this); + permissionsHelper.addMemberToRole(USER, "Editor", PermissionsHelper.MemberType.user); + setInitialPassword(USER); + } + + /* + Regression coverage for Secure Issue 51523: Invalidate sessions on password change + */ + @Test + public void testSessionInvalidatesAfterPasswordChange() throws IOException + { + signOut(); + signIn(USER); + Connection cn = createDefaultConnection(); + SelectRowsResponse response; + SelectRowsCommand selectCmd = new SelectRowsCommand("auditLog", "UserAuditEvent"); + try + { + response = selectCmd.execute(cn, getProjectName()); + Assert.assertEquals("Did not establish the database connection before the password change", 200, + response.getStatusCode()); + } + catch (IOException | CommandException e) + { + throw new RuntimeException(e); + } + + log("Changing the user password"); + String newPassword = PasswordUtil.getPassword() + "&*&*"; + goToChangePassword().setOldPassword(PasswordUtil.getPassword()) + .setPassword1(newPassword) + .setPassword2(newPassword) + .clickSubmit(); + + log("Verifying the session is invalid"); + try + { + selectCmd.execute(cn, getProjectName()); + fail("Test should have thrown command exception"); + } + catch (CommandException e) + { + Assert.assertEquals("Session should be expired", 401, e.getStatusCode()); + } + signOut(); + } + + /* + Regression coverage for Secure Issue 31493: Test for session and cookie persistence through login and logout + */ + @Test + public void testCookieAndSessionFromLogout() throws IOException + { + goToProjectHome(); + + log("Capture the cookie after login"); + Set beforeCookie = getDriver().manage().getCookies(); + + log("Establish the connection"); + Connection cn = createDefaultConnection(); + SelectRowsResponse response; + SelectRowsCommand selectCmd = new SelectRowsCommand("auditLog", "UserAuditEvent"); + try + { + response = selectCmd.execute(cn, getProjectName()); + Assert.assertEquals("Did not establish the database connection before the password change", 200, + response.getStatusCode()); + } + catch (CommandException e) + { + throw new RuntimeException(e); + } + + log("Sign out"); + signOut(); + + log("Verify previously established connection be used after log out"); + try + { + selectCmd.execute(cn, getProjectName()); + fail("Should fail when logged out"); + } + catch (CommandException e) + { + Assert.assertEquals("Command execution should fail when logged out", HttpStatus.SC_UNAUTHORIZED, e.getStatusCode()); + } + + log("Capture the cookie after logout"); + Set afterCookie = getDriver().manage().getCookies(); + Assert.assertFalse("Before and after log out cookie should be different", beforeCookie.equals(afterCookie)); + } + + private SetPasswordForm goToChangePassword() + { + if (PasswordUtil.getUsername().equals(getCurrentUser())) + throw new IllegalArgumentException("Don't change the primary site admin user's password"); + + goToMyAccount(); + clickButton("Change Password"); + return new SetPasswordForm(getDriver()); + } + + @Override + protected String getProjectName() + { + return "InvalidateSessionTest Project"; + } + + @Override + public List getAssociatedModules() + { + return Arrays.asList(); + } +} From 14d72e51119d3dea88aab9fb478fd903328e40c0 Mon Sep 17 00:00:00 2001 From: labkey-sweta Date: Fri, 17 Jan 2025 14:12:54 -0800 Subject: [PATCH 2/5] Code review changes --- .../test/pages/user/UserDetailsPage.java | 12 ++ src/org/labkey/test/tests/ApiKeyTest.java | 133 ++++++++---------- .../test/tests/InvalidateSessionTest.java | 41 ++---- .../test/tests/core/login/PasswordTest.java | 16 +-- 4 files changed, 90 insertions(+), 112 deletions(-) diff --git a/src/org/labkey/test/pages/user/UserDetailsPage.java b/src/org/labkey/test/pages/user/UserDetailsPage.java index 72c9ecf386..3a6cd52bde 100644 --- a/src/org/labkey/test/pages/user/UserDetailsPage.java +++ b/src/org/labkey/test/pages/user/UserDetailsPage.java @@ -3,7 +3,9 @@ import org.labkey.test.Locator; import org.labkey.test.WebDriverWrapper; import org.labkey.test.WebTestHelper; +import org.labkey.test.components.core.login.SetPasswordForm; import org.labkey.test.pages.LabKeyPage; +import org.labkey.test.util.PasswordUtil; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; @@ -34,6 +36,15 @@ public ClonePermissionsPage clickClonePermission() return new ClonePermissionsPage(getDriver()); } + public SetPasswordForm clickChangePassword() + { + if (PasswordUtil.getUsername().equals(getCurrentUser())) + throw new IllegalArgumentException("Don't change the primary site admin user's password"); + + clickAndWait(elementCache().changePwdButton); + return new SetPasswordForm(getDriver()); + } + @Override protected ElementCache newElementCache() { @@ -44,5 +55,6 @@ protected class ElementCache extends LabKeyPage.ElementCache { WebElement editButton = Locator.lkButton("Edit").findWhenNeeded(this); WebElement cloneButton = Locator.lkButton("Clone Permissions").findWhenNeeded(this); + WebElement changePwdButton = Locator.lkButton("Change Password").findWhenNeeded(this); } } diff --git a/src/org/labkey/test/tests/ApiKeyTest.java b/src/org/labkey/test/tests/ApiKeyTest.java index 05bfc89380..f778dee9c0 100644 --- a/src/org/labkey/test/tests/ApiKeyTest.java +++ b/src/org/labkey/test/tests/ApiKeyTest.java @@ -75,7 +75,7 @@ public class ApiKeyTest extends BaseWebDriverTest @BeforeClass public static void setupProject() { - ApiKeyTest init = (ApiKeyTest) getCurrentTest(); + ApiKeyTest init = getCurrentTest(); init.doSetup(); } @@ -107,74 +107,21 @@ public void testSessionKey() throws IOException String apiKey = generateSessionKey(); - verifyValidAPIKey(apiKey); + verifyValidAPIKey(createApiKeyConnection(apiKey, false)); log("Verify session key remains valid if key generation is turned off"); goToAdminConsole() .clickSiteSettings() .setAllowSessionKeys(false) .save(); - verifyValidAPIKey(apiKey); + verifyValidAPIKey(createApiKeyConnection(apiKey, false)); signOut(); log("Verify that logging out invalidates session keys"); - verifyInvalidAPIKey(apiKey, null); + verifyInvalidAPIKey(createApiKeyConnection(apiKey, false), false); simpleSignIn(); log("Verify that session keys remain invalid after logging back in"); - verifyInvalidAPIKey(apiKey, null); - } - - private void verifyValidAPIKey(String apiKey) throws IOException - { - verifyValidAPIKey(apiKey, false, null); - } - - private Connection verifyValidAPIKey(String apiKey, boolean basicAuth, @Nullable Connection connection) throws IOException - { - Connection cn; - if (connection == null) - cn = new Connection(WebTestHelper.getBaseURL(), basicAuth ? new BasicAuthCredentialsProvider(API_USERNAME, apiKey) - : new ApiKeyCredentialsProvider(apiKey)); - else - cn = connection; - try - { - GetSchemasCommand cmd = new GetSchemasCommand(); - GetSchemasResponse resp = cmd.execute(cn, getProjectName()); - List schemaNames = resp.getSchemaNames().stream().map(String::toLowerCase).collect(Collectors.toList()); - Set missingSchemas = new HashSet<>(Arrays.asList("pipeline", "lists", "core")); - missingSchemas.removeAll(schemaNames); - assertTrue("Some expected schemas missing. Schemas missing: " + missingSchemas, missingSchemas.isEmpty()); - } - catch (CommandException e) - { - throw new RuntimeException("Response: " + e.getStatusCode(), e); - } - return cn; - } - - private void verifyInvalidAPIKey(String apiKey, @Nullable Connection connection) throws IOException - { - boolean isSessionKey = !apiKey.startsWith(API_USERNAME); - Connection cn; - if (connection == null) - cn = new Connection(WebTestHelper.getBaseURL(), new ApiKeyCredentialsProvider(apiKey)); - else - cn = connection; - try - { - GetSchemasCommand cmd = new GetSchemasCommand(); - cmd.execute(cn, getProjectName()); - if (isSessionKey) - fail("Session key didn't invalidate after logout"); - else - fail("API key should no longer be valid"); - } - catch (CommandException e) - { - assertEquals("Wrong response for invalid " + (isSessionKey ? "session" : "API") + " key", HttpStatus.SC_UNAUTHORIZED, e.getStatusCode()); - log("Success: command failed as expected."); - } + verifyInvalidAPIKey(createApiKeyConnection(apiKey, false), false); } @Test @@ -192,14 +139,14 @@ public void testNonAdminUser() throws IOException signIn(EDITOR_USER.getEmail(), EDITOR_USER.getPassword()); String keyDescription = "Key for editing"; String apiKey = generateAPIKey(keyDescription); - verifyValidAPIKey(apiKey); + verifyValidAPIKey(createApiKeyConnection(apiKey, false)); QueryGrid grid = new QueryGrid.QueryGridFinder(getDriver()).waitFor(); int beforeDeleteCount = grid.getRecordCount(); assertFalse("Row with description not found", grid.getRowMap("Description", keyDescription).isEmpty()); grid = deleteAPIKeyViaUI(); assertEquals("Number of keys after UI deletion not as expected", beforeDeleteCount - 1, grid.getRecordCount()); - verifyInvalidAPIKey(apiKey, null); + verifyInvalidAPIKey(createApiKeyConnection(apiKey, false), false); } @Test @@ -215,9 +162,9 @@ public void testStandardApiKey() throws IOException String apiKey = generateAPIKeyAndRecord(_generatedApiKeys); log("Verify active API key via api authentication"); - verifyValidAPIKey(apiKey); + verifyValidAPIKey(createApiKeyConnection(apiKey, false)); log("Verify active API key via basic authentication"); - verifyValidAPIKey(apiKey, true, null); + verifyValidAPIKey(createApiKeyConnection(apiKey, true)); log("Generate two other keys for use in testing deletion."); generateAPIKey(null); @@ -232,7 +179,7 @@ public void testStandardApiKey() throws IOException .clickSiteSettings() .setAllowApiKeys(false) .save(); - verifyValidAPIKey(apiKey); + verifyValidAPIKey(createApiKeyConnection(apiKey, false)); log("Verify key deletion via UI with disabled api key generation works."); grid = deleteAPIKeyViaUI(); @@ -242,7 +189,7 @@ public void testStandardApiKey() throws IOException log("Verify revoked/deleted api key"); deleteAPIKeys(_generatedApiKeys); - verifyInvalidAPIKey(apiKey, null); + verifyInvalidAPIKey(createApiKeyConnection(apiKey, false), false); } /* @@ -261,16 +208,17 @@ public void testSessionInvalidatesAfterAPIKeyChange() throws IOException .save(); String apiKey1 = generateAPIKeyAndRecord(_generatedApiKeys); - Connection cn = verifyValidAPIKey(apiKey1, false, null); + Connection cn = createApiKeyConnection(apiKey1, false); + verifyValidAPIKey(cn); log("Deleting the apikey"); deleteAPIKeys(_generatedApiKeys); log("Verifying the session associated with deleted apikey is invalid"); - verifyInvalidAPIKey(apiKey1, cn); +// verifyInvalidAPIKey(cn, false); log("Verifying that new connection cannot be created after apikey is deleted"); - verifyInvalidAPIKey(apiKey1, null); + verifyInvalidAPIKey(createApiKeyConnection(apiKey1, false), false); log("Generating the apikey which expires in ten seconds"); goToAdminConsole() @@ -279,17 +227,17 @@ public void testSessionInvalidatesAfterAPIKeyChange() throws IOException .setApiKeyExpiration(CustomizeSitePage.KeyExpirationOptions.TEN_SECONDS) .save(); - log("Verify apikey generation fails"); + log("Verify apikey expiration"); goToExternalToolPage(); String apikey2 = ApiKeyPanel.panelFinder(getDriver()).find().generateApiKey(); - log("Verify valid apikey cannot be created before expiring"); - verifyValidAPIKey(apikey2, false, null); + log("Verify apikey can be used before expiring"); + verifyValidAPIKey(createApiKeyConnection(apikey2, false)); sleep(10000); // Wait for apikey to expire - log("Verify apikey cannot be created after the timer is expired"); - verifyInvalidAPIKey(apikey2, null); + log("Verify apikey cannot be used after it has expired"); + verifyInvalidAPIKey(createApiKeyConnection(apikey2, false), false); } @Test @@ -392,6 +340,47 @@ public void testSessionKeyDisabled() throws IOException } } + private void verifyValidAPIKey(Connection connection) throws IOException + { + try + { + GetSchemasCommand cmd = new GetSchemasCommand(); + GetSchemasResponse resp = cmd.execute(connection, getProjectName()); + List schemaNames = resp.getSchemaNames().stream().map(String::toLowerCase).collect(Collectors.toList()); + Set missingSchemas = new HashSet<>(Arrays.asList("pipeline", "lists", "core")); + missingSchemas.removeAll(schemaNames); + assertTrue("Some expected schemas missing. Schemas missing: " + missingSchemas, missingSchemas.isEmpty()); + } + catch (CommandException e) + { + throw new RuntimeException("Response: " + e.getStatusCode(), e); + } + } + + private Connection createApiKeyConnection(String apiKey, boolean basicAuth) + { + return new Connection(WebTestHelper.getBaseURL(), basicAuth ? new BasicAuthCredentialsProvider(API_USERNAME, apiKey) + : new ApiKeyCredentialsProvider(apiKey)); + } + + private void verifyInvalidAPIKey(Connection connection, boolean isSessionKey) throws IOException + { + try + { + GetSchemasCommand cmd = new GetSchemasCommand(); + cmd.execute(connection, getProjectName()); + if (isSessionKey) + fail("Session key didn't invalidate after logout"); + else + fail("API key should no longer be valid"); + } + catch (CommandException e) + { + assertEquals("Wrong response for invalid " + (isSessionKey ? "session" : "API") + " key", HttpStatus.SC_UNAUTHORIZED, e.getStatusCode()); + log("Success: command failed as expected."); + } + } + private void verifyAPIKeysTablePresence(boolean isAdmin) { beginAt(new URLBuilder("query", "begin", getProjectName()).setFragment("sbh-ssp-core").buildURL()); diff --git a/src/org/labkey/test/tests/InvalidateSessionTest.java b/src/org/labkey/test/tests/InvalidateSessionTest.java index 826f723dda..da15af2632 100644 --- a/src/org/labkey/test/tests/InvalidateSessionTest.java +++ b/src/org/labkey/test/tests/InvalidateSessionTest.java @@ -1,6 +1,7 @@ package org.labkey.test.tests; import org.apache.hc.core5.http.HttpStatus; +import org.jetbrains.annotations.Nullable; import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; @@ -11,7 +12,6 @@ import org.labkey.remoteapi.query.SelectRowsResponse; import org.labkey.test.BaseWebDriverTest; import org.labkey.test.categories.Daily; -import org.labkey.test.components.core.login.SetPasswordForm; import org.labkey.test.util.ApiPermissionsHelper; import org.labkey.test.util.PasswordUtil; import org.labkey.test.util.PermissionsHelper; @@ -20,9 +20,9 @@ import java.io.IOException; import java.util.Arrays; import java.util.List; -import java.util.Set; import static org.junit.Assert.fail; +import static org.labkey.test.WebTestHelper.getCookies; @Category({Daily.class}) public class InvalidateSessionTest extends BaseWebDriverTest @@ -36,20 +36,24 @@ public static void setupProject() init.doSetup(); } + @Override + protected @Nullable String getProjectName() + { + return null; + } + @Override protected void doCleanup(boolean afterTest) { - _containerHelper.deleteProject(getProjectName(), afterTest); _userHelper.deleteUsers(afterTest, USER); } private void doSetup() { - _containerHelper.createProject(getProjectName(), null); _userHelper.createUser(USER); ApiPermissionsHelper permissionsHelper = new ApiPermissionsHelper(this); - permissionsHelper.addMemberToRole(USER, "Editor", PermissionsHelper.MemberType.user); + permissionsHelper.addMemberToRole(USER, "Folder Administrator", PermissionsHelper.MemberType.user); setInitialPassword(USER); } @@ -66,7 +70,7 @@ public void testSessionInvalidatesAfterPasswordChange() throws IOException SelectRowsCommand selectCmd = new SelectRowsCommand("auditLog", "UserAuditEvent"); try { - response = selectCmd.execute(cn, getProjectName()); + response = selectCmd.execute(cn, "Home"); Assert.assertEquals("Did not establish the database connection before the password change", 200, response.getStatusCode()); } @@ -77,7 +81,8 @@ public void testSessionInvalidatesAfterPasswordChange() throws IOException log("Changing the user password"); String newPassword = PasswordUtil.getPassword() + "&*&*"; - goToChangePassword().setOldPassword(PasswordUtil.getPassword()) + goToMyAccount().clickChangePassword() + .setOldPassword(PasswordUtil.getPassword()) .setPassword1(newPassword) .setPassword2(newPassword) .clickSubmit(); @@ -101,10 +106,8 @@ public void testSessionInvalidatesAfterPasswordChange() throws IOException @Test public void testCookieAndSessionFromLogout() throws IOException { - goToProjectHome(); - log("Capture the cookie after login"); - Set beforeCookie = getDriver().manage().getCookies(); + Cookie beforeCookie = getCookies(getCurrentUser()).get(Connection.JSESSIONID); log("Establish the connection"); Connection cn = createDefaultConnection(); @@ -136,26 +139,10 @@ public void testCookieAndSessionFromLogout() throws IOException } log("Capture the cookie after logout"); - Set afterCookie = getDriver().manage().getCookies(); + Cookie afterCookie = getCookies(getCurrentUser()).get(Connection.JSESSIONID); Assert.assertFalse("Before and after log out cookie should be different", beforeCookie.equals(afterCookie)); } - private SetPasswordForm goToChangePassword() - { - if (PasswordUtil.getUsername().equals(getCurrentUser())) - throw new IllegalArgumentException("Don't change the primary site admin user's password"); - - goToMyAccount(); - clickButton("Change Password"); - return new SetPasswordForm(getDriver()); - } - - @Override - protected String getProjectName() - { - return "InvalidateSessionTest Project"; - } - @Override public List getAssociatedModules() { diff --git a/src/org/labkey/test/tests/core/login/PasswordTest.java b/src/org/labkey/test/tests/core/login/PasswordTest.java index b3c55d2362..f72d2e7149 100644 --- a/src/org/labkey/test/tests/core/login/PasswordTest.java +++ b/src/org/labkey/test/tests/core/login/PasswordTest.java @@ -138,7 +138,7 @@ public void testStrongPassword() assertSignedInNotImpersonating(); impersonate(USER); - SetPasswordForm changePasswordForm = goToChangePassword(); + SetPasswordForm changePasswordForm = goToMyAccount().clickChangePassword(); log("Verify strength gauge for 'ChangePasswordAction'"); changePasswordForm.verifyPasswordStrengthGauge(USER, displayName); @@ -183,7 +183,7 @@ public void testReusePassword() assertTextNotPresent("Choose a new password."); } // fail, used 9 passwords ago. - goToChangePassword() + goToMyAccount().clickChangePassword() .setOldPassword(currentPassword) .setNewPassword(VERY_STRONG_PASSWORD + 1) .clickSubmitExpectingError("Your password must not match a recently used password."); @@ -357,19 +357,9 @@ protected String setInitialPassword(int userId, String password) @LogMethod (quiet = true) protected void changePassword(String oldPassword, @LoggedParam String password) { - goToChangePassword() + goToMyAccount().clickChangePassword() .setOldPassword(oldPassword) .setNewPassword(password) .clickSubmit(); } - - private SetPasswordForm goToChangePassword() - { - if (PasswordUtil.getUsername().equals(getCurrentUser())) - throw new IllegalArgumentException("Don't change the primary site admin user's password"); - - goToMyAccount(); - clickButton("Change Password"); - return new SetPasswordForm(getDriver()); - } } From 182dcbdc863586147f7171f670fb5ce37fef5373 Mon Sep 17 00:00:00 2001 From: labkey-sweta Date: Fri, 17 Jan 2025 14:31:49 -0800 Subject: [PATCH 3/5] Test coverage for Issue 52004: Session associated with APIKey can used even after APIKey is deleted. --- src/org/labkey/test/tests/ApiKeyTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/org/labkey/test/tests/ApiKeyTest.java b/src/org/labkey/test/tests/ApiKeyTest.java index f778dee9c0..6293e4af9f 100644 --- a/src/org/labkey/test/tests/ApiKeyTest.java +++ b/src/org/labkey/test/tests/ApiKeyTest.java @@ -214,8 +214,11 @@ public void testSessionInvalidatesAfterAPIKeyChange() throws IOException log("Deleting the apikey"); deleteAPIKeys(_generatedApiKeys); + /* + Regression coverage for Issue 52004: Session associated with APIKey can used even after APIKey is deleted. + */ log("Verifying the session associated with deleted apikey is invalid"); -// verifyInvalidAPIKey(cn, false); + verifyInvalidAPIKey(cn, false); log("Verifying that new connection cannot be created after apikey is deleted"); verifyInvalidAPIKey(createApiKeyConnection(apiKey1, false), false); From 260c8f65ef1dc6a89ef55451f7974461968391a2 Mon Sep 17 00:00:00 2001 From: labkey-sweta Date: Fri, 24 Jan 2025 12:21:39 -0800 Subject: [PATCH 4/5] Code review changes --- .../test/tests/InvalidateSessionTest.java | 43 ++++------ .../test/tests/UserPermissionsTest.java | 85 ++++++++++++++----- 2 files changed, 83 insertions(+), 45 deletions(-) diff --git a/src/org/labkey/test/tests/InvalidateSessionTest.java b/src/org/labkey/test/tests/InvalidateSessionTest.java index da15af2632..1320cdbeda 100644 --- a/src/org/labkey/test/tests/InvalidateSessionTest.java +++ b/src/org/labkey/test/tests/InvalidateSessionTest.java @@ -20,9 +20,9 @@ import java.io.IOException; import java.util.Arrays; import java.util.List; +import java.util.Set; import static org.junit.Assert.fail; -import static org.labkey.test.WebTestHelper.getCookies; @Category({Daily.class}) public class InvalidateSessionTest extends BaseWebDriverTest @@ -61,23 +61,16 @@ private void doSetup() Regression coverage for Secure Issue 51523: Invalidate sessions on password change */ @Test - public void testSessionInvalidatesAfterPasswordChange() throws IOException + public void testSessionInvalidatesAfterPasswordChange() throws IOException, CommandException { signOut(); signIn(USER); Connection cn = createDefaultConnection(); SelectRowsResponse response; SelectRowsCommand selectCmd = new SelectRowsCommand("auditLog", "UserAuditEvent"); - try - { - response = selectCmd.execute(cn, "Home"); - Assert.assertEquals("Did not establish the database connection before the password change", 200, - response.getStatusCode()); - } - catch (IOException | CommandException e) - { - throw new RuntimeException(e); - } + response = selectCmd.execute(cn, "Home"); + Assert.assertEquals("Did not establish the database connection before the password change", 200, + response.getStatusCode()); log("Changing the user password"); String newPassword = PasswordUtil.getPassword() + "&*&*"; @@ -104,25 +97,18 @@ public void testSessionInvalidatesAfterPasswordChange() throws IOException Regression coverage for Secure Issue 31493: Test for session and cookie persistence through login and logout */ @Test - public void testCookieAndSessionFromLogout() throws IOException + public void testCookieAndSessionFromLogout() throws IOException, CommandException { log("Capture the cookie after login"); - Cookie beforeCookie = getCookies(getCurrentUser()).get(Connection.JSESSIONID); + Set beforeCookie = getDriver().manage().getCookies(); log("Establish the connection"); Connection cn = createDefaultConnection(); SelectRowsResponse response; SelectRowsCommand selectCmd = new SelectRowsCommand("auditLog", "UserAuditEvent"); - try - { - response = selectCmd.execute(cn, getProjectName()); - Assert.assertEquals("Did not establish the database connection before the password change", 200, - response.getStatusCode()); - } - catch (CommandException e) - { - throw new RuntimeException(e); - } + response = selectCmd.execute(cn, getProjectName()); + Assert.assertEquals("Did not establish the database connection before the password change", 200, + response.getStatusCode()); log("Sign out"); signOut(); @@ -139,8 +125,13 @@ public void testCookieAndSessionFromLogout() throws IOException } log("Capture the cookie after logout"); - Cookie afterCookie = getCookies(getCurrentUser()).get(Connection.JSESSIONID); - Assert.assertFalse("Before and after log out cookie should be different", beforeCookie.equals(afterCookie)); + Set afterCookie = getDriver().manage().getCookies(); + Assert.assertFalse("Before and after log out cookie should be different", getJSessionIdValue(beforeCookie).equals(getJSessionIdValue(afterCookie))); + } + + private String getJSessionIdValue(Set cookies) + { + return cookies.stream().toList().get(1).getValue(); } @Override diff --git a/src/org/labkey/test/tests/UserPermissionsTest.java b/src/org/labkey/test/tests/UserPermissionsTest.java index ce52536fc4..fff70ffd1a 100644 --- a/src/org/labkey/test/tests/UserPermissionsTest.java +++ b/src/org/labkey/test/tests/UserPermissionsTest.java @@ -16,12 +16,17 @@ package org.labkey.test.tests; +import org.junit.Assert; +import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; import org.labkey.test.BaseWebDriverTest; import org.labkey.test.Locator; import org.labkey.test.TestTimeoutException; import org.labkey.test.categories.Daily; +import org.labkey.test.pages.core.admin.ShowAuditLogPage; +import org.labkey.test.util.ApiPermissionsHelper; +import org.labkey.test.util.DataRegionTable; import org.labkey.test.util.LogMethod; import org.labkey.test.util.PortalHelper; import org.openqa.selenium.WebElement; @@ -36,14 +41,13 @@ @BaseWebDriverTest.ClassTimeout(minutes = 7) public class UserPermissionsTest extends BaseWebDriverTest { - PortalHelper portalHelper = new PortalHelper(this); protected static final String PERM_PROJECT_NAME = "PermissionCheckProject"; protected static final String DENIED_SUB_FOLDER_NAME = "UnlinkedFolder"; protected static final String GAMMA_SUB_FOLDER_NAME = "GammaFolder"; protected static final String GAMMA_EDITOR_GROUP_NAME = "GammaEditor"; protected static final String GAMMA_AUTHOR_GROUP_NAME = "GammaAuthor"; protected static final String GAMMA_READER_GROUP_NAME = "GammaReader"; -// protected static final String GAMMA_RESTRICTED_READER_GROUP_NAME = "GammaRestrictedReader"; + // protected static final String GAMMA_RESTRICTED_READER_GROUP_NAME = "GammaRestrictedReader"; protected static final String GAMMA_SUBMITTER_GROUP_NAME = "GammaSubmitter"; protected static final String GAMMA_ADMIN_GROUP_NAME = "GammaAdmin"; //permissions @@ -54,11 +58,20 @@ public class UserPermissionsTest extends BaseWebDriverTest protected static final String GAMMA_AUTHOR_PAGE_TITLE = "This is a Test Message from : " + GAMMA_AUTHOR_USER; protected static final String GAMMA_READER_USER = "gammareader@security.test"; protected static final String GAMMA_PROJECT_ADMIN_USER = "gammaadmin@security.test"; + protected static final String GAMMA_SUBMITTER_USER = "gammasubmitter@security.test"; + PortalHelper portalHelper = new PortalHelper(this); //I can't really find any docs on what this is exactly? // protected static final String GAMMA_RESTRICTED_READER_USER = "gammarestricted@security.test"; // protected static final String GAMMA_SUBMITTER_USER = "gammasubmitter@security.test"; + @BeforeClass + public static void setupProject() + { + UserPermissionsTest init = getCurrentTest(); + init.doSetup(); + } + @Override public List getAssociatedModules() { @@ -83,23 +96,10 @@ protected void doCleanup(boolean afterTest) throws TestTimeoutException log(this.getClass().getName() + " Cleaning Up"); _containerHelper.deleteProject(PERM_PROJECT_NAME, afterTest); - deleteUsersIfPresent(GAMMA_EDITOR_USER, GAMMA_AUTHOR_USER, GAMMA_READER_USER, GAMMA_PROJECT_ADMIN_USER); + deleteUsersIfPresent(GAMMA_EDITOR_USER, GAMMA_AUTHOR_USER, GAMMA_READER_USER, GAMMA_PROJECT_ADMIN_USER, GAMMA_SUBMITTER_USER); } - @Test - public void testSteps() - { - enableEmailRecorder(); - userPermissionRightsTest(); - } - - /** - * Create some projects, create some groups, permissions for those groups - * Create some users, assign to groups and validate the permissions by - * impersonating the user. - */ - @LogMethod - private void userPermissionRightsTest() + private void doSetup() { _containerHelper.createProject(PERM_PROJECT_NAME, null); _permissionsHelper.createPermissionsGroup(GAMMA_EDITOR_GROUP_NAME); @@ -107,8 +107,8 @@ private void userPermissionRightsTest() _permissionsHelper.setPermissions(GAMMA_EDITOR_GROUP_NAME, "Editor"); createUserInProjectForGroup(GAMMA_EDITOR_USER, PERM_PROJECT_NAME, GAMMA_EDITOR_GROUP_NAME, false); - _containerHelper.createSubfolder(PERM_PROJECT_NAME, PERM_PROJECT_NAME, DENIED_SUB_FOLDER_NAME, "None", new String[] {"Messages", "Wiki"}, true); - _containerHelper.createSubfolder(PERM_PROJECT_NAME, DENIED_SUB_FOLDER_NAME, GAMMA_SUB_FOLDER_NAME, "None", new String[] {"Messages", "Wiki"}, true); + _containerHelper.createSubfolder(PERM_PROJECT_NAME, PERM_PROJECT_NAME, DENIED_SUB_FOLDER_NAME, "None", new String[]{"Messages", "Wiki"}, true); + _containerHelper.createSubfolder(PERM_PROJECT_NAME, DENIED_SUB_FOLDER_NAME, GAMMA_SUB_FOLDER_NAME, "None", new String[]{"Messages", "Wiki"}, true); portalHelper.addWebPart("Messages"); assertElementPresent(Locator.linkWithText("Messages")); portalHelper.addWebPart("Wiki"); @@ -123,6 +123,7 @@ private void userPermissionRightsTest() _permissionsHelper.assertPermissionSetting(GAMMA_READER_GROUP_NAME, "No Permissions"); _permissionsHelper.setPermissions(GAMMA_READER_GROUP_NAME, "Reader"); createUserInProjectForGroup(GAMMA_READER_USER, PERM_PROJECT_NAME, GAMMA_READER_GROUP_NAME, false); + //Create Author User clickProject(PERM_PROJECT_NAME); _permissionsHelper.enterPermissionsUI(); @@ -130,17 +131,36 @@ private void userPermissionRightsTest() _permissionsHelper.assertPermissionSetting(GAMMA_AUTHOR_GROUP_NAME, "No Permissions"); _permissionsHelper.setPermissions(GAMMA_AUTHOR_GROUP_NAME, "Author"); createUserInProjectForGroup(GAMMA_AUTHOR_USER, PERM_PROJECT_NAME, GAMMA_AUTHOR_GROUP_NAME, false); + //Create the Submitter User clickProject(PERM_PROJECT_NAME); _permissionsHelper.enterPermissionsUI(); _permissionsHelper.createPermissionsGroup(GAMMA_SUBMITTER_GROUP_NAME); _permissionsHelper.assertPermissionSetting(GAMMA_SUBMITTER_GROUP_NAME, "No Permissions"); _permissionsHelper.setPermissions(GAMMA_SUBMITTER_GROUP_NAME, "Submitter"); + // TODO: Add submitter to a group /* * I need a way to test submitter, I can't even view a folder where submitter has permissions when * impersonating on my local labkey, so may require special page? */ + } + + @Test + public void testSteps() + { + enableEmailRecorder(); + userPermissionRightsTest(); + } + + /** + * Create some projects, create some groups, permissions for those groups + * Create some users, assign to groups and validate the permissions by + * impersonating the user. + */ + @LogMethod + private void userPermissionRightsTest() + { //Make sure the Editor can edit impersonate(GAMMA_EDITOR_USER); @@ -236,6 +256,33 @@ private void userPermissionRightsTest() signIn(); } + /* + Regression for Secure Issue 51187: Additional automation testing for group audit logs + */ + @Test + public void testAuditLogForGroupUpdates() + { + ApiPermissionsHelper permissionsHelper = new ApiPermissionsHelper(this); + + log("Add user to the group and verify logs"); + _userHelper.createUser(GAMMA_SUBMITTER_USER); + permissionsHelper.addUserToProjGroup(GAMMA_SUBMITTER_USER, getProjectName(), GAMMA_SUBMITTER_GROUP_NAME); + verifyAuditLog("User: " + GAMMA_SUBMITTER_USER + " was added as a member to Group: " + GAMMA_SUBMITTER_GROUP_NAME); + + log("Remove user from group and verify logs"); + goToProjectHome(); + permissionsHelper.removeUserFromGroup(GAMMA_SUBMITTER_GROUP_NAME, GAMMA_SUBMITTER_USER); + verifyAuditLog("User: " + GAMMA_SUBMITTER_USER + " was deleted from Group: " + GAMMA_SUBMITTER_GROUP_NAME); + } + + private void verifyAuditLog(String expectedComment) + { + ShowAuditLogPage showAuditLogPage = goToAdminConsole().clickAuditLog(); + showAuditLogPage.selectView("Group and role events"); + DataRegionTable table = showAuditLogPage.getLogTable(); + Assert.assertEquals("Incorrect audit log record for user getting added to group", expectedComment, table.getDataAsText(0, "Comment")); + } + private void clickLinkWithTextNoTarget(String text) { String href = getAttribute(Locator.linkWithText(text), "href"); From be1b10c680fb615a4e1ca1b6382ec46b5e58eab5 Mon Sep 17 00:00:00 2001 From: labkey-sweta Date: Fri, 24 Jan 2025 13:59:03 -0800 Subject: [PATCH 5/5] Code review updates --- .../test/tests/InvalidateSessionTest.java | 8 +++++++- .../labkey/test/tests/UserPermissionsTest.java | 18 +++++------------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/org/labkey/test/tests/InvalidateSessionTest.java b/src/org/labkey/test/tests/InvalidateSessionTest.java index 1320cdbeda..a172f95f43 100644 --- a/src/org/labkey/test/tests/InvalidateSessionTest.java +++ b/src/org/labkey/test/tests/InvalidateSessionTest.java @@ -131,7 +131,13 @@ public void testCookieAndSessionFromLogout() throws IOException, CommandExceptio private String getJSessionIdValue(Set cookies) { - return cookies.stream().toList().get(1).getValue(); + String jsession = ""; + for (Cookie c : cookies) + { + if(c.getName().equals(Connection.JSESSIONID)) + jsession = c.getValue(); + } + return jsession; } @Override diff --git a/src/org/labkey/test/tests/UserPermissionsTest.java b/src/org/labkey/test/tests/UserPermissionsTest.java index fff70ffd1a..3884a8b083 100644 --- a/src/org/labkey/test/tests/UserPermissionsTest.java +++ b/src/org/labkey/test/tests/UserPermissionsTest.java @@ -146,21 +146,13 @@ private void doSetup() */ } + /* + Validate the permissions by impersonating the user. + */ @Test - public void testSteps() + public void testUserPermissionRights() { enableEmailRecorder(); - userPermissionRightsTest(); - } - - /** - * Create some projects, create some groups, permissions for those groups - * Create some users, assign to groups and validate the permissions by - * impersonating the user. - */ - @LogMethod - private void userPermissionRightsTest() - { //Make sure the Editor can edit impersonate(GAMMA_EDITOR_USER); @@ -280,7 +272,7 @@ private void verifyAuditLog(String expectedComment) ShowAuditLogPage showAuditLogPage = goToAdminConsole().clickAuditLog(); showAuditLogPage.selectView("Group and role events"); DataRegionTable table = showAuditLogPage.getLogTable(); - Assert.assertEquals("Incorrect audit log record for user getting added to group", expectedComment, table.getDataAsText(0, "Comment")); + Assert.assertEquals("Incorrect audit log record for group events", expectedComment, table.getDataAsText(0, "Comment")); } private void clickLinkWithTextNoTarget(String text)