Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rate limiter refactor test #13994

Merged
merged 16 commits into from
Apr 24, 2017
2 changes: 1 addition & 1 deletion framework/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ Yii Framework 2 Change Log

2.0.12 under development
--------------------------

- Enh: Test write for yii\filters\RateLimiter and refactoring (vladis84)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enh #13994: Refactored yii\filters\RateLimiter. Added tests (vladis84)

- Enh #13820: Add new HTTP status code 451 (yyxx9988)
- Bug #13671: Fixed error handler trace to work correctly with XDebug (samdark)
- Bug #13657: Fixed `yii\helpers\StringHelper::truncateHtml()` skip extra tags at the end (sam002)
Expand Down
27 changes: 18 additions & 9 deletions framework/filters/RateLimiter.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,26 +61,35 @@ class RateLimiter extends ActionFilter
*/
public $response;

public function init()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpdoc missing

Copy link
Contributor Author

@vladis84 vladis84 Apr 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected

{
if (!$this->request) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these initializations for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/yiisoft/yii2/blob/master/framework/filters/RateLimiter.php#L75
It is difficult to say why in the filter these properties

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test for === null instead of !.

$this->request = Yii::$app->getRequest();
}

if (!$this->response) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test for === null instead of !.

$this->response = Yii::$app->getResponse();
}
}

/**
* @inheritdoc
*/
public function beforeAction($action)
{
$user = $this->user ? : (Yii::$app->getUser() ? Yii::$app->getUser()->getIdentity(false) : null);
if ($user instanceof RateLimitInterface) {
if ($this->user === null && Yii::$app->getUser()) {
$this->user = Yii::$app->getUser()->getIdentity(false);
}

if ($this->user instanceof RateLimitInterface) {
Yii::trace('Check rate limit', __METHOD__);
$this->checkRateLimit(
$user,
$this->request ? : Yii::$app->getRequest(),
$this->response ? : Yii::$app->getResponse(),
$action
);
} elseif ($user) {
$this->checkRateLimit($this->user, $this->request, $this->response, $action);
} elseif ($this->user) {
Yii::info('Rate limit skipped: "user" does not implement RateLimitInterface.', __METHOD__);
} else {
Yii::info('Rate limit skipped: user not logged in.', __METHOD__);
}

return true;
}

Expand Down
164 changes: 164 additions & 0 deletions tests/framework/filters/RateLimiterTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
<?php

namespace yiiunit\framework\filters;

use Yii;
use yiiunit\TestCase;
use Prophecy\Argument;
use yiiunit\framework\filters\stubs\UserIdentity;
use yii\web\User;
use yii\web\Request;
use yii\web\Response;
use yii\log\Logger;
use yii\filters\RateLimiter;

/**
* @group filters
*/
class RateLimiterTest extends TestCase
{
protected function setUp()
{
parent::setUp();

/* @var $logger Logger|\Prophecy\ObjectProphecy */
$logger = $this->prophesize(Logger::className());
$logger
->log(Argument::any(), Argument::any(), Argument::any())
->will(function ($parameters, $logger) {
$logger->messages = $parameters;
});

Yii::setLogger($logger->reveal());

$this->mockWebApplication();
}
protected function tearDown()
{
parent::tearDown();
Yii::setLogger(null);
}

public function testInitFilledRequest()
{
$rateLimiter = new RateLimiter(['request' => 'Request']);

$this->assertEquals('Request', $rateLimiter->request);
}

public function testInitNotFilledRequest()
{
$rateLimiter = new RateLimiter();

$this->assertInstanceOf(Request::className(), $rateLimiter->request);
}

public function testInitFilledResponse()
{
$rateLimiter = new RateLimiter(['response' => 'Response']);

$this->assertEquals('Response', $rateLimiter->response);
}

public function testInitNotFilledResponse()
{
$rateLimiter = new RateLimiter();

$this->assertInstanceOf(Response::className(), $rateLimiter->response);
}

public function testBeforeActionUserInstanceOfRateLimitInterface()
{
/* @var $rateLimiter RateLimiter|PHPUnit_Framework_MockObject_MockObject */
$rateLimiter = $this->getMockBuilder(RateLimiter::className())
->setMethods(['checkRateLimit'])
->getMock();
$rateLimiter->expects(self::at(0))
->method('checkRateLimit')
->willReturn(true);
$rateLimiter->user = new UserIdentity;

$result = $rateLimiter->beforeAction('test');

$this->assertContains('Check rate limit', Yii::getLogger()->messages);
$this->assertTrue($result);
}

public function testBeforeActionUserNotInstanceOfRateLimitInterface()
{
$rateLimiter = new RateLimiter(['user' => 'User']);

$result = $rateLimiter->beforeAction('test');

$this->assertContains('Rate limit skipped: "user" does not implement RateLimitInterface.', Yii::getLogger()->messages);
$this->assertTrue($result);
}

public function testBeforeActionEmptyUser()
{
$user = new User(['identityClass' => UserIdentity::className()]);
Yii::$app->set('user', $user);
$rateLimiter = new RateLimiter();

$result = $rateLimiter->beforeAction('test');

$this->assertContains('Rate limit skipped: user not logged in.', Yii::getLogger()->messages);
$this->assertTrue($result);
}

public function testCheckRateLimitTooManyRequests()
{
/* @var $user UserIdentity|\Prophecy\ObjectProphecy */
$user = $this->prophesize(UserIdentity::className());
$user->getRateLimit(Argument::any(), Argument::any())
->willReturn([1, 1]);
$user->loadAllowance(Argument::any(), Argument::any())
->willReturn([1, time() + 2]);
$user->saveAllowance(Argument::any(), Argument::any(), Argument::any(), Argument::any())
->willReturn(null);
$rateLimiter = new RateLimiter();

$this->setExpectedException('yii\web\TooManyRequestsHttpException');
$rateLimiter->checkRateLimit($user->reveal(), Yii::$app->request, Yii::$app->response, 'testAction');
}

public function testCheckRateaddRateLimitHeaders()
{
/* @var $user UserIdentity|\Prophecy\ObjectProphecy */
$user = $this->prophesize(UserIdentity::className());
$user->getRateLimit(Argument::any(), Argument::any())
->willReturn([1, 1]);
$user->loadAllowance(Argument::any(), Argument::any())
->willReturn([1, time()]);
$user->saveAllowance(Argument::any(), Argument::any(), Argument::any(), Argument::any())
->willReturn(null);
$rateLimiter = $this->getMockBuilder(RateLimiter::className())
->setMethods(['addRateLimitHeaders'])
->getMock();
$rateLimiter->expects(self::at(0))
->method('addRateLimitHeaders')
->willReturn(null);

$rateLimiter->checkRateLimit($user->reveal(), Yii::$app->request, Yii::$app->response, 'testAction');
}

public function testAddRateLimitHeadersDisabledRateLimitHeaders()
{
$rateLimiter = new RateLimiter();
$rateLimiter->enableRateLimitHeaders = false;
$response = Yii::$app->response;

$rateLimiter->addRateLimitHeaders($response, 1, 0, 0);
$this->assertCount(0, $response->getHeaders());
}

public function testAddRateLimitHeadersEnabledRateLimitHeaders()
{
$rateLimiter = new RateLimiter();
$rateLimiter->enableRateLimitHeaders = true;
$response = Yii::$app->response;

$rateLimiter->addRateLimitHeaders($response, 1, 0, 0);
$this->assertCount(3, $response->getHeaders());
}
}
18 changes: 17 additions & 1 deletion tests/framework/filters/stubs/UserIdentity.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@

use yii\base\Component;
use yii\base\NotSupportedException;
use yii\filters\RateLimitInterface;
use yii\web\IdentityInterface;

/**
* Class UserIdentity
* @author Dmitry Naumenko <d.naumenko.a@gmail.com>
* @since 2.0.7
*/
class UserIdentity extends Component implements IdentityInterface
class UserIdentity extends Component implements IdentityInterface, RateLimitInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected

{
private static $ids = [
'user1',
Expand Down Expand Up @@ -63,4 +64,19 @@ public function validateAuthKey($authKey)
{
throw new NotSupportedException();
}

public function getRateLimit($request, $action)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like a good idea to me. Interface should return an array in this case but it does not. Implementing methods like that isn't right. Better to leave default user implementation w/o this interface at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, the api will be broken.
The programmer must implement the RatLimitInterface for the use of RateLimiter.
The structure is reliable than the array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it be broken in case of returning nulls as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected

{

}

public function loadAllowance($request, $action)
{

}

public function saveAllowance($request, $action, $allowance, $timestamp)
{

}
}