Skip to content

Commit

Permalink
Fix rate limiter failing /manage page test
Browse files Browse the repository at this point in the history
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
  • Loading branch information
AuroraLS3 committed May 18, 2024
1 parent 8a16455 commit 3dc6ac5
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 */ }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,14 @@ 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;
@Untrusted Request request = null;
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)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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 <https://www.gnu.org/licenses/>.
*/
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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -206,6 +209,8 @@ static Stream<Arguments> 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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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");
}
Expand Down
3 changes: 2 additions & 1 deletion Plan/common/src/test/java/extension/SeleniumExtension.java
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down

0 comments on commit 3dc6ac5

Please sign in to comment.