bug symfony#51666 [RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens (10n)

This PR was merged into the 6.3 branch.

Discussion
----------

[RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

CompoundLimiter is accepting requests when the limit was reached previously.

When processing the limiters and the first one consumes exactly all the remaining tokens (remaining=0, accepted=true) and the next one already reached the limit previously (remaining=0, accepted=0) the $minimalRateLimit is considered the first one that will accept the request (even if it's not the most restrictive).

For example:
CompoundLimiter includes 2 limiters:
- limiter 1 - remaining 2 tokens
- limiter 2 - remaining 0 tokens

After consuming 2 tokens each each limiter generates to limits:
- `limiter1`->consume(2), generates a limit indicating `0` remaining tokens, **accepts** the request (it was last permitted)
- `limiter2`->consume(2), generates a limit indicating `0` remaining tokens, **did not accept** the request (it did not have 2 tokens to satisfy the request)

Because both of them have at this moment `0` remaining tokens, the minimum limit that is returned will be the limit from the `limiter1` . This means that the CompundLimiter will accept the request, even if the `limiter2` should be more restrictive.

If we switch the order in the constructor, the request will be denied.  The order should not matter.

Commits
-------

65ce7f8 [RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens

Author: nicolas-grekas

src/Symfony/Component/RateLimiter/CompoundLimiter.php

@@ -42,7 +42,11 @@ public function consume(int $tokens = 1): RateLimit
         foreach ($this->limiters as $limiter) {
             $rateLimit = $limiter->consume($tokens);
 
-            if (null === $minimalRateLimit || $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()) {
+            if (
+                null === $minimalRateLimit
+                || $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()
+                || ($minimalRateLimit->isAccepted() && !$rateLimit->isAccepted())
+            ) {
                 $minimalRateLimit = $rateLimit;
             }
         }

src/Symfony/Component/RateLimiter/Tests/CompoundLimiterTest.php

@@ -36,19 +36,49 @@ public function testConsume()
     {
         $limiter1 = $this->createLimiter(4, new \DateInterval('PT1S'));
         $limiter2 = $this->createLimiter(8, new \DateInterval('PT10S'));
-        $limiter3 = $this->createLimiter(12, new \DateInterval('PT30S'));
+        $limiter3 = $this->createLimiter(16, new \DateInterval('PT30S'));
         $limiter = new CompoundLimiter([$limiter1, $limiter2, $limiter3]);
 
-        $this->assertEquals(0, $limiter->consume(4)->getRemainingTokens(), 'Limiter 1 reached the limit');
+        $rateLimit = $limiter->consume(4);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 1 reached the limit');
+        $this->assertTrue($rateLimit->isAccepted(), 'All limiters accept (exact limit on limiter 1)');
+
+        $rateLimit = $limiter->consume(1);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 1 reached the limit');
+        $this->assertFalse($rateLimit->isAccepted(), 'Limiter 1 did not accept limit');
+
         sleep(1); // reset limiter1's window
-        $this->assertTrue($limiter->consume(3)->isAccepted());
 
-        $this->assertEquals(0, $limiter->consume()->getRemainingTokens(), 'Limiter 2 has no remaining tokens left');
-        sleep(10); // reset limiter2's window
-        $this->assertTrue($limiter->consume(3)->isAccepted());
+        $rateLimit = $limiter->consume(3);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 2 consumed exactly the remaining tokens');
+        $this->assertTrue($rateLimit->isAccepted(), 'All accept the request (exact limit on limiter 2)');
+
+        $rateLimit = $limiter->consume(1);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 2 had remaining tokens left');
+        $this->assertFalse($rateLimit->isAccepted(), 'Limiter 2 did not accept the request');
+
+        sleep(1); // reset limiter1's window again,  to make sure that the limiter2 overrides limiter1
+
+        // make sure to consume all allowed by limiter1, limiter2 already had 0 remaining
+        $rateLimit = $limiter->consume(4);
+        $this->assertEquals(
+            0,
+            $rateLimit->getRemainingTokens(),
+            'Limiter 1 consumed the remaining tokens (accept), Limiter 2 did not have any remaining (not accept)'
+        );
+        $this->assertFalse($rateLimit->isAccepted(), 'Limiter 2 reached the limit already');
+
+        sleep(10); // reset limiter2's window (also limiter1)
+
+        $rateLimit = $limiter->consume(3);
+        $this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 3 had exactly 3 tokens   (accept)');
+        $this->assertTrue($rateLimit->isAccepted());
+
+        $rateLimit = $limiter->consume(1);
+        $this->assertFalse($rateLimit->isAccepted(), 'Limiter 3 reached the limit previously');
+
+        sleep(30); // reset limiter3's window (also limiter1 and limiter2)
 
-        $this->assertEquals(0, $limiter->consume()->getRemainingTokens(), 'Limiter 3 reached the limit');
-        sleep(20); // reset limiter3's window
         $this->assertTrue($limiter->consume()->isAccepted());
     }