Adjust logic to store period instead of current timestamp

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

4 files changed, 30 insertions, 44 deletions

diff --git a/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php b/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php
index a7f2a7b4b62..3b7382d747e 100644
--- a/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php
+++ b/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php
@@ -71,21 +71,28 @@ class DatabaseBackend implements IBackend {
 	 * @throws \OCP\DB\Exception
 	 */
 	private function getExistingAttemptCount(
-		string $identifier,
-		int $seconds
+		string $identifier
 	): int {
+		$currentTime = $this->timeFactory->getDateTime();
+
 		$qb = $this->dbConnection->getQueryBuilder();
-		$notOlderThan = $this->timeFactory->getDateTime()->sub(new \DateInterval("PT{$seconds}S"));
+		$qb->delete(self::TABLE_NAME)
+			->where(
+				$qb->expr()->lte('delete_after', $qb->createParameter('currentTime'))
+			)
+			->setParameter('currentTime', $currentTime, 'datetime')
+			->executeStatement();
 
+		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->selectAlias($qb->createFunction('COUNT(*)'), 'count')
 			->from(self::TABLE_NAME)
 			->where(
 				$qb->expr()->eq('hash', $qb->createNamedParameter($identifier, IQueryBuilder::PARAM_STR))
 			)
 			->andWhere(
-				$qb->expr()->gte('timestamp', $qb->createParameter('notOlderThan'))
+				$qb->expr()->gte('delete_after', $qb->createParameter('currentTime'))
 			)
-			->setParameter('notOlderThan', $notOlderThan, 'datetime');
+			->setParameter('currentTime', $currentTime, 'datetime');
 
 		$cursor = $qb->executeQuery();
 		$row = $cursor->fetch();
@@ -98,10 +105,9 @@ class DatabaseBackend implements IBackend {
 	 * {@inheritDoc}
 	 */
 	public function getAttempts(string $methodIdentifier,
-								string $userIdentifier,
-								int $seconds): int {
+								string $userIdentifier): int {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
-		return $this->getExistingAttemptCount($identifier, $seconds);
+		return $this->getExistingAttemptCount($identifier);
 	}
 
 	/**
@@ -111,25 +117,14 @@ class DatabaseBackend implements IBackend {
 									string $userIdentifier,
 									int $period) {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
-		$currentTime = $this->timeFactory->getDateTime();
-		$notOlderThan = $this->timeFactory->getDateTime('@' . $period);
+		$deleteAfter = $this->timeFactory->getDateTime()->add(new \DateInterval("PT{$period}S"));
 
 		$qb = $this->dbConnection->getQueryBuilder();
 
-		$qb->delete(self::TABLE_NAME)
-			->where(
-				$qb->expr()->eq('hash', $qb->createNamedParameter($identifier, IQueryBuilder::PARAM_STR))
-			)
-			->andWhere(
-				$qb->expr()->lt('timestamp', $qb->createParameter('notOlderThan'))
-			)
-			->setParameter('notOlderThan', $notOlderThan, 'datetime')
-			->executeStatement();
-
 		$qb->insert(self::TABLE_NAME)
 			->values([
 				'hash' => $qb->createNamedParameter($identifier, IQueryBuilder::PARAM_STR),
-				'timestamp' => $qb->createNamedParameter($currentTime, IQueryBuilder::PARAM_DATE),
+				'delete_after' => $qb->createNamedParameter($deleteAfter, IQueryBuilder::PARAM_DATE),
 			])
 			->executeStatement();
 	}
diff --git a/lib/private/Security/RateLimiting/Backend/IBackend.php b/lib/private/Security/RateLimiting/Backend/IBackend.php
index d87f53311b2..835a97993da 100644
--- a/lib/private/Security/RateLimiting/Backend/IBackend.php
+++ b/lib/private/Security/RateLimiting/Backend/IBackend.php
@@ -35,16 +35,14 @@ namespace OC\Security\RateLimiting\Backend;
  */
 interface IBackend {
 	/**
-	 * Gets the amount of attempts within the last specified seconds
+	 * Gets the amount of attempts for the specified method
 	 *
 	 * @param string $methodIdentifier Identifier for the method
 	 * @param string $userIdentifier Identifier for the user
-	 * @param int $seconds Seconds to look back at
 	 * @return int
 	 */
 	public function getAttempts(string $methodIdentifier,
-								string $userIdentifier,
-								int $seconds): int;
+								string $userIdentifier): int;
 
 	/**
 	 * Registers an attempt
diff --git a/lib/private/Security/RateLimiting/Backend/MemoryCache.php b/lib/private/Security/RateLimiting/Backend/MemoryCacheBackend.php
index 0dab25e4048..f4880fb239c 100644
--- a/lib/private/Security/RateLimiting/Backend/MemoryCache.php
+++ b/lib/private/Security/RateLimiting/Backend/MemoryCacheBackend.php
@@ -33,12 +33,12 @@ use OCP\ICache;
 use OCP\ICacheFactory;
 
 /**
- * Class MemoryCache uses the configured distributed memory cache for storing
+ * Class MemoryCacheBackend uses the configured distributed memory cache for storing
  * rate limiting data.
  *
  * @package OC\Security\RateLimiting\Backend
  */
-class MemoryCache implements IBackend {
+class MemoryCacheBackend implements IBackend {
 	/** @var ICache */
 	private $cache;
 	/** @var ITimeFactory */
@@ -86,16 +86,14 @@ class MemoryCache implements IBackend {
 	 * {@inheritDoc}
 	 */
 	public function getAttempts(string $methodIdentifier,
-								string $userIdentifier,
-								int $seconds): int {
+								string $userIdentifier): int {
 		$identifier = $this->hash($methodIdentifier, $userIdentifier);
 		$existingAttempts = $this->getExistingAttempts($identifier);
 
 		$count = 0;
 		$currentTime = $this->timeFactory->getTime();
-		/** @var array $existingAttempts */
-		foreach ($existingAttempts as $attempt) {
-			if (($attempt + $seconds) > $currentTime) {
+		foreach ($existingAttempts as $expirationTime) {
+			if ($expirationTime > $currentTime) {
 				$count++;
 			}
 		}
@@ -113,16 +111,16 @@ class MemoryCache implements IBackend {
 		$existingAttempts = $this->getExistingAttempts($identifier);
 		$currentTime = $this->timeFactory->getTime();
 
-		// Unset all attempts older than $period
-		foreach ($existingAttempts as $key => $attempt) {
-			if (($attempt + $period) < $currentTime) {
+		// Unset all attempts that are already expired
+		foreach ($existingAttempts as $key => $expirationTime) {
+			if ($expirationTime < $currentTime) {
 				unset($existingAttempts[$key]);
 			}
 		}
 		$existingAttempts = array_values($existingAttempts);
 
 		// Store the new attempt
-		$existingAttempts[] = (string)$currentTime;
+		$existingAttempts[] = (string)($currentTime + $period);
 		$this->cache->set($identifier, json_encode($existingAttempts));
 	}
 }
diff --git a/lib/private/Security/RateLimiting/Limiter.php b/lib/private/Security/RateLimiting/Limiter.php
index ede72e887fc..1c14fce2a55 100644
--- a/lib/private/Security/RateLimiting/Limiter.php
+++ b/lib/private/Security/RateLimiting/Limiter.php
@@ -35,17 +35,12 @@ use OCP\IUser;
 class Limiter {
 	/** @var IBackend */
 	private $backend;
-	/** @var ITimeFactory */
-	private $timeFactory;
 
 	/**
-	 * @param ITimeFactory $timeFactory
 	 * @param IBackend $backend
 	 */
-	public function __construct(ITimeFactory $timeFactory,
-								IBackend $backend) {
+	public function __construct(IBackend $backend) {
 		$this->backend = $backend;
-		$this->timeFactory = $timeFactory;
 	}
 
 	/**
@@ -59,12 +54,12 @@ class Limiter {
 							  string $userIdentifier,
 							  int $period,
 							  int $limit): void {
-		$existingAttempts = $this->backend->getAttempts($methodIdentifier, $userIdentifier, $period);
+		$existingAttempts = $this->backend->getAttempts($methodIdentifier, $userIdentifier);
 		if ($existingAttempts >= $limit) {
 			throw new RateLimitExceededException();
 		}
 
-		$this->backend->registerAttempt($methodIdentifier, $userIdentifier, $this->timeFactory->getTime());
+		$this->backend->registerAttempt($methodIdentifier, $userIdentifier, $period);
 	}
 
 	/**