From 3dc6ac5afa0bd4c66863ccddbf8a13a21586ede7 Mon Sep 17 00:00:00 2001 From: Aurora Lahtela <24460436+AuroraLS3@users.noreply.github.com> Date: Sat, 18 May 2024 10:46:49 +0300 Subject: [PATCH] Fix rate limiter failing /manage page test Poor implementation makes /manage/groups page request /v1/permissions n times, where n is amount of groups. Since each visibility test registers a new group, there are a lot of them when opening the manage page, leading to tripping the rate-limiter --- .../delivery/webserver/RateLimitGuard.java | 19 +++----- .../webserver/http/RequestHandler.java | 3 +- .../commands/RemoveWebGroupsTransaction.java | 43 +++++++++++++++++++ .../AccessControlVisibilityTest.java | 13 ++++-- .../java/extension/SeleniumExtension.java | 3 +- .../hooks/context/groupEditContextHook.jsx | 1 + 6 files changed, 62 insertions(+), 20 deletions(-) create mode 100644 Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/commands/RemoveWebGroupsTransaction.java diff --git a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/RateLimitGuard.java b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/RateLimitGuard.java index af2eb0de49..f893e42afb 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/RateLimitGuard.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/RateLimitGuard.java @@ -40,14 +40,7 @@ public class RateLimitGuard { .expireAfterWrite(120, TimeUnit.SECONDS) .build(); - public boolean shouldPreventRequest(@Untrusted String accessor) { - Integer attempts = requests.getIfPresent(accessor); - if (attempts == null) return false; - // Too many attempts, forbid further attempts. - return attempts >= ATTEMPT_LIMIT; - } - - public void increaseAttemptCount(@Untrusted String requestPath, @Untrusted String accessor) { + public boolean shouldPreventRequest(@Untrusted String requestPath, @Untrusted String accessor) { String previous = lastRequestPath.getIfPresent(accessor); if (!Objects.equals(previous, requestPath)) { resetAttemptCount(accessor); @@ -60,23 +53,23 @@ public void increaseAttemptCount(@Untrusted String requestPath, @Untrusted Strin lastRequestPath.put(accessor, requestPath); requests.put(accessor, attempts + 1); + + // Too many attempts, forbid further attempts. + return attempts + 1 >= ATTEMPT_LIMIT; } public void resetAttemptCount(@Untrusted String accessor) { // previous request changed - requests.cleanUp(); requests.invalidate(accessor); + requests.cleanUp(); } public static class Disabled extends RateLimitGuard { @Override - public boolean shouldPreventRequest(String accessor) { + public boolean shouldPreventRequest(@Untrusted String requestedPath, String accessor) { return false; } - @Override - public void increaseAttemptCount(String requestPath, String accessor) { /* Disabled */ } - @Override public void resetAttemptCount(String accessor) { /* Disabled */ } } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/http/RequestHandler.java b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/http/RequestHandler.java index 8da7f59d36..3cb1a99954 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/http/RequestHandler.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/http/RequestHandler.java @@ -58,7 +58,6 @@ public RequestHandler(WebserverConfiguration webserverConfiguration, ResponseFac public Response getResponse(InternalRequest internalRequest) { @Untrusted String accessAddress = internalRequest.getAccessAddress(webserverConfiguration); @Untrusted String requestedPath = internalRequest.getRequestedPath(); - rateLimitGuard.increaseAttemptCount(requestedPath, accessAddress); boolean blocked = false; Response response; @@ -66,7 +65,7 @@ public Response getResponse(InternalRequest internalRequest) { if (bruteForceGuard.shouldPreventRequest(accessAddress)) { response = responseFactory.failedLoginAttempts403(); blocked = true; - } else if (rateLimitGuard.shouldPreventRequest(accessAddress)) { + } else if (rateLimitGuard.shouldPreventRequest(requestedPath, accessAddress)) { response = responseFactory.failedRateLimit403(); blocked = true; } else if (!webserverConfiguration.getAllowedIpList().isAllowed(accessAddress)) { diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/commands/RemoveWebGroupsTransaction.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/commands/RemoveWebGroupsTransaction.java new file mode 100644 index 0000000000..7cf4ccb1f5 --- /dev/null +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/commands/RemoveWebGroupsTransaction.java @@ -0,0 +1,43 @@ +/* + * This file is part of Player Analytics (Plan). + * + * Plan is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License v3 as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Plan is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Plan. If not, see . + */ +package com.djrapitops.plan.storage.database.transactions.commands; + +import com.djrapitops.plan.storage.database.sql.tables.webuser.SecurityTable; +import com.djrapitops.plan.storage.database.sql.tables.webuser.WebGroupTable; +import com.djrapitops.plan.storage.database.sql.tables.webuser.WebGroupToPermissionTable; +import com.djrapitops.plan.storage.database.sql.tables.webuser.WebUserPreferencesTable; +import com.djrapitops.plan.storage.database.transactions.Transaction; + +/** + * Transaction that removes all web groups from the database. + * + * @author AuroraLS3 + */ +public class RemoveWebGroupsTransaction extends Transaction { + + @Override + protected void performOperations() { + clearTable(WebUserPreferencesTable.TABLE_NAME); + clearTable(SecurityTable.TABLE_NAME); + clearTable(WebGroupToPermissionTable.TABLE_NAME); + clearTable(WebGroupTable.TABLE_NAME); + } + + private void clearTable(String tableName) { + execute("DELETE FROM " + tableName); + } +} \ No newline at end of file diff --git a/Plan/common/src/test/java/com/djrapitops/plan/delivery/webserver/AccessControlVisibilityTest.java b/Plan/common/src/test/java/com/djrapitops/plan/delivery/webserver/AccessControlVisibilityTest.java index 544619633e..aa1f89ed31 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/delivery/webserver/AccessControlVisibilityTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/delivery/webserver/AccessControlVisibilityTest.java @@ -30,6 +30,7 @@ import com.djrapitops.plan.storage.database.Database; import com.djrapitops.plan.storage.database.queries.objects.ServerQueries; import com.djrapitops.plan.storage.database.transactions.StoreServerInformationTransaction; +import com.djrapitops.plan.storage.database.transactions.commands.RemoveWebGroupsTransaction; import com.djrapitops.plan.storage.database.transactions.commands.StoreWebUserTransaction; import com.djrapitops.plan.storage.database.transactions.events.StoreServerPlayerTransaction; import com.djrapitops.plan.storage.database.transactions.webuser.StoreWebGroupTransaction; @@ -94,6 +95,8 @@ static void setUp(PlanSystem system, @TempDir Path tempDir, PlanConfig config) t @AfterEach void tearDownTest(WebDriver driver) { + String address = "https://localhost:" + TEST_PORT_NUMBER + "/auth/logout"; + driver.get(address); SeleniumExtension.newTab(driver); driver.manage().deleteAllCookies(); } @@ -206,6 +209,8 @@ static Stream pageLevelVisibleCases() { @ParameterizedTest(name = "Access with visibility {0} can see element #{1} in /{2}") @MethodSource("pageLevelVisibleCases") void pageVisible(WebPermission permission, String element, String page, Database database, ServerUUID serverUUID, ChromeDriver driver) throws Exception { + // TODO Remove after fixing manage/groups making bazillion calls to /v1/permissions + database.executeTransaction(new RemoveWebGroupsTransaction()).get(); User user = registerUser(database, permission); String address = "https://localhost:" + TEST_PORT_NUMBER + "/" + page; @@ -285,7 +290,7 @@ void serverPageElementNotVisible(WebPermission permission, String element, Strin driver.get(address); login(driver, user); - SeleniumExtension.waitForElementToBeVisible(By.id("wrapper"), driver); + SeleniumExtension.waitForElementToBeVisible(By.className("login-username"), driver); By id = By.id(element); assertThrows(NoSuchElementException.class, () -> driver.findElement(id), () -> "Saw element #" + element + " at " + address + " without permission to"); assertNoLogs(driver, address); @@ -327,7 +332,7 @@ void networkPageElementNotVisible(WebPermission permission, String element, Stri driver.get(address); login(driver, user); - SeleniumExtension.waitForElementToBeVisible(By.id("wrapper"), driver); + SeleniumExtension.waitForElementToBeVisible(By.className("login-username"), driver); By id = By.id(element); assertThrows(NoSuchElementException.class, () -> driver.findElement(id), () -> "Saw element #" + element + " at " + address + " without permission to"); assertNoLogs(driver, address); @@ -360,7 +365,7 @@ void playerPageElementNotVisible(WebPermission permission, String element, Strin driver.get(address); login(driver, user); - SeleniumExtension.waitForElementToBeVisible(By.id("wrapper"), driver); + SeleniumExtension.waitForElementToBeVisible(By.className("login-username"), driver); By id = By.id(element); assertThrows(NoSuchElementException.class, () -> driver.findElement(id), () -> "Saw element #" + element + " at " + address + " without permission to"); assertNoLogs(driver, address); @@ -406,7 +411,7 @@ void playerSelfNonVisibilityTests(Database database, ServerUUID serverUUID, Chro driver.get(address); login(driver, playerUser); - SeleniumExtension.waitForElementToBeVisible(By.id("wrapper"), driver); + SeleniumExtension.waitForElementToBeVisible(By.className("login-username"), driver); By id = By.id(element); assertThrows(NoSuchElementException.class, () -> driver.findElement(id), () -> "Saw element #" + element + " at /player/" + TestConstants.PLAYER_TWO_UUID + " without permission to"); } diff --git a/Plan/common/src/test/java/extension/SeleniumExtension.java b/Plan/common/src/test/java/extension/SeleniumExtension.java index f1c95599d1..e664888d62 100644 --- a/Plan/common/src/test/java/extension/SeleniumExtension.java +++ b/Plan/common/src/test/java/extension/SeleniumExtension.java @@ -57,7 +57,8 @@ public static void waitForElementToBeVisible(By by, ChromeDriver driver) { SeleniumExtension.waitForPageLoadForSeconds(5, driver); Awaitility.await("waitForElementToBeVisible " + by.toString()) .atMost(5, TimeUnit.SECONDS) - .ignoreException(NoSuchElementException.class) + .ignoreExceptionsMatching(throwable -> throwable instanceof NoSuchElementException + || throwable instanceof StaleElementReferenceException) .until(() -> driver.findElement(by).isDisplayed()); } diff --git a/Plan/react/dashboard/src/hooks/context/groupEditContextHook.jsx b/Plan/react/dashboard/src/hooks/context/groupEditContextHook.jsx index b669534c8a..bd78085bcb 100644 --- a/Plan/react/dashboard/src/hooks/context/groupEditContextHook.jsx +++ b/Plan/react/dashboard/src/hooks/context/groupEditContextHook.jsx @@ -51,6 +51,7 @@ export const GroupEditContextProvider = ({groupName, children}) => { const [allPermissions, setAllPermissions] = useState([]); useEffect(() => { + // TODO Make this only happen once when opening groups page fetchAvailablePermissions().then(response => { setAllPermissions(response?.data?.permissions); });