tonyair/silverstripe-versionfeed
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-versionfeed synced 2024-10-22 11:05:31 +02:00
Code Issues Projects Releases Wiki Activity

FIX Separate tests, ensure versioned cache mode does not interfere

Browse Source
This commit is contained in:
Robbie Averill 2018-06-18 22:54:33 +12:00
parent d48fab6faa
commit bd59ce6200
2 changed files with 65 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
src/Filters/RateLimitFilter.php
View File

@ -5,13 +5,14 @@ namespace SilverStripe\VersionFeed\Filters;
use SilverStripe\Core\Config\Config; use SilverStripe\Core\Config\Config;
use SilverStripe\Control\Controller; use SilverStripe\Control\Controller;
use SilverStripe\Control\HTTPResponse_Exception; use SilverStripe\Control\HTTPResponse_Exception;
use SilverStripe\Versioned\Versioned;
/** /**
* Provides rate limiting of execution of a callback * Provides rate limiting of execution of a callback
*/ */
class RateLimitFilter extends ContentFilter class RateLimitFilter extends ContentFilter
{ {
/** /**
* Time duration (in second) to allow for generation of cached results. Requests to * Time duration (in second) to allow for generation of cached results. Requests to
* pages that within this time period that do not hit the cache (and would otherwise trigger * pages that within this time period that do not hit the cache (and would otherwise trigger
@ -21,7 +22,7 @@ class RateLimitFilter extends ContentFilter
* @var int * @var int
*/ */
private static $lock_timeout = 5; private static $lock_timeout = 5;
/** /**
* Determine if the cache generation should be locked on a per-page basis. If true, concurrent page versions * Determine if the cache generation should be locked on a per-page basis. If true, concurrent page versions
* may be generated without rate interference. * may be generated without rate interference.
@ -30,7 +31,7 @@ class RateLimitFilter extends ContentFilter
* @var bool * @var bool
*/ */
private static $lock_bypage = false; private static $lock_bypage = false;
/** /**
* Determine if rate limiting should be applied independently to each IP address. This method is not * Determine if rate limiting should be applied independently to each IP address. This method is not
* reliable, as most DDoS attacks use multiple IP addresses. * reliable, as most DDoS attacks use multiple IP addresses.
@ -49,12 +50,12 @@ class RateLimitFilter extends ContentFilter
* @var int * @var int
*/ */
private static $lock_cooldown = 2; private static $lock_cooldown = 2;
/** /**
* Cache key prefix * Cache key prefix
*/ */
const CACHE_PREFIX = 'RateLimitBegin'; const CACHE_PREFIX = 'RateLimitBegin';
/** /**
* Determines the key to use for saving the current rate * Determines the key to use for saving the current rate
* *
@ -64,18 +65,18 @@ class RateLimitFilter extends ContentFilter
protected function getCacheKey($itemkey) protected function getCacheKey($itemkey)
{ {
$key = self::CACHE_PREFIX; $key = self::CACHE_PREFIX;
// Add global identifier // Add global identifier
if ($this->config()->get('lock_bypage')) { if ($this->config()->get('lock_bypage')) {
$key .= '_' . md5($itemkey); $key .= '_' . md5($itemkey);
} }
// Add user-specific identifier // Add user-specific identifier
if ($this->config()->get('lock_byuserip') && Controller::has_curr()) { if ($this->config()->get('lock_byuserip') && Controller::has_curr()) {
$ip = Controller::curr()->getRequest()->getIP(); $ip = Controller::curr()->getRequest()->getIP();
$key .= '_' . md5($ip); $key .= '_' . md5($ip);
} }
return $key; return $key;
} }
@ -87,7 +88,7 @@ class RateLimitFilter extends ContentFilter
if (isset($_GET['flush']) || !$timeout) { if (isset($_GET['flush']) || !$timeout) {
return parent::getContent($key, $callback); return parent::getContent($key, $callback);
} }
// Generate result with rate limiting enabled // Generate result with rate limiting enabled
$limitKey = $this->getCacheKey($key); $limitKey = $this->getCacheKey($key);
$cache = $this->getCache(); $cache = $this->getCache();
@ -101,10 +102,10 @@ class RateLimitFilter extends ContentFilter
} }
$lifetime = Config::inst()->get(ContentFilter::class, 'cache_lifetime') ?: null; $lifetime = Config::inst()->get(ContentFilter::class, 'cache_lifetime') ?: null;
// Apply rate limit // Apply rate limit
$cache->set($limitKey, time() + $timeout, $lifetime); $cache->set($limitKey, time() + $timeout, $lifetime);
// Generate results // Generate results
$result = parent::getContent($key, $callback); $result = parent::getContent($key, $callback);

102
tests/VersionFeedFunctionalTest.php
View File

@ -3,9 +3,11 @@
namespace SilverStripe\VersionFeed\Tests; namespace SilverStripe\VersionFeed\Tests;
use Page; use Page;
use PageController;
use Psr\SimpleCache\CacheInterface; use Psr\SimpleCache\CacheInterface;
use SilverStripe\CMS\Model\SiteTree; use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Control\Director; use SilverStripe\Control\Director;
use SilverStripe\Core\Cache\CacheFactory;
use SilverStripe\Core\Config\Config; use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\FunctionalTest; use SilverStripe\Dev\FunctionalTest;
@ -19,26 +21,31 @@ use SilverStripe\VersionFeed\VersionFeedController;
class VersionFeedFunctionalTest extends FunctionalTest class VersionFeedFunctionalTest extends FunctionalTest
{ {
protected $usesDatabase = true; protected $usesDatabase = true;
protected $baseURI = 'http://www.fakesite.test'; protected $baseURI = 'http://www.fakesite.test';
protected static $required_extensions = array( protected static $required_extensions = [
'Page' => array(VersionFeed::class), Page::class => [VersionFeed::class],
'PageController' => array(VersionFeedController::class), PageController::class => [VersionFeedController::class],
); ];
protected $userIP; protected $userIP;
/**
* @var CacheInterface
*/
protected $cache;
protected function setUp() protected function setUp()
{ {
Director::config()->set('alternate_base_url', $this->baseURI); Director::config()->set('alternate_base_url', $this->baseURI);
parent::setUp(); parent::setUp();
$cache = Injector::inst()->get( $this->cache = Injector::inst()->get(
CacheInterface::class . '.VersionFeedController' CacheInterface::class . '.VersionFeedController'
); );
$cache->clear(); $this->cache->clear();
$this->userIP = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : null; $this->userIP = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : null;
@ -52,20 +59,23 @@ class VersionFeedFunctionalTest extends FunctionalTest
Config::modify()->set(RateLimitFilter::class, 'lock_bypage', false); Config::modify()->set(RateLimitFilter::class, 'lock_bypage', false);
Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', false); Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', false);
Config::modify()->set(RateLimitFilter::class, 'lock_cooldown', false); Config::modify()->set(RateLimitFilter::class, 'lock_cooldown', false);
// Ensure any version based caches read from the live cache
Versioned::set_reading_mode(Versioned::DEFAULT_MODE);
} }
protected function tearDown() protected function tearDown()
{ {
Director::config()->set('alternate_base_url', null); Director::config()->set('alternate_base_url', null);
$_SERVER['REMOTE_ADDR'] = $this->userIP; $_SERVER['REMOTE_ADDR'] = $this->userIP;
parent::tearDown(); parent::tearDown();
} }
public function testPublicHistory() public function testPublicHistoryPublicHistoryDisabled()
{ {
$page = $this->createPageWithChanges(array('PublicHistory' => false)); $page = $this->createPageWithChanges(['PublicHistory' => false]);
$response = $this->get($page->RelativeLink('changes')); $response = $this->get($page->RelativeLink('changes'));
$this->assertEquals( $this->assertEquals(
@ -81,8 +91,11 @@ class VersionFeedFunctionalTest extends FunctionalTest
(bool)$xml->channel->item, (bool)$xml->channel->item,
'With Page\'s "PublicHistory" disabled, `allchanges` action should not have an item in the channel' 'With Page\'s "PublicHistory" disabled, `allchanges` action should not have an item in the channel'
); );
}
$page = $this->createPageWithChanges(array('PublicHistory' => true)); public function testPublicHistoryPublicHistoryEnabled()
{
$page = $this->createPageWithChanges(['PublicHistory' => true]);
$response = $this->get($page->RelativeLink('changes')); $response = $this->get($page->RelativeLink('changes'));
$this->assertEquals(200, $response->getStatusCode()); $this->assertEquals(200, $response->getStatusCode());
@ -107,15 +120,11 @@ class VersionFeedFunctionalTest extends FunctionalTest
Config::modify()->set(RateLimitFilter::class, 'lock_timeout', 20); Config::modify()->set(RateLimitFilter::class, 'lock_timeout', 20);
Config::modify()->set(CachedContentFilter::class, 'cache_enabled', true); Config::modify()->set(CachedContentFilter::class, 'cache_enabled', true);
$page1 = $this->createPageWithChanges(array('PublicHistory' => true, 'Title' => 'Page1')); $page1 = $this->createPageWithChanges(['PublicHistory' => true, 'Title' => 'Page1']);
$page2 = $this->createPageWithChanges(array('PublicHistory' => true, 'Title' => 'Page2')); $page2 = $this->createPageWithChanges(['PublicHistory' => true, 'Title' => 'Page2']);
// Artifically set cache lock // Artifically set cache lock
Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', false); $this->cache->set(RateLimitFilter::CACHE_PREFIX, time() + 10);
$cache = Injector::inst()->get(
CacheInterface::class . '.VersionFeedController'
);
$cache->set(RateLimitFilter::CACHE_PREFIX, time() + 10);
// Test normal hit // Test normal hit
$response = $this->get($page1->RelativeLink('changes')); $response = $this->get($page1->RelativeLink('changes'));
@ -127,13 +136,13 @@ class VersionFeedFunctionalTest extends FunctionalTest
// Test page specific lock // Test page specific lock
Config::modify()->set(RateLimitFilter::class, 'lock_bypage', true); Config::modify()->set(RateLimitFilter::class, 'lock_bypage', true);
$key = implode('_', array( $key = implode('_', [
'changes', 'changes',
$page1->ID, $page1->ID,
Versioned::get_versionnumber_by_stage(SiteTree::class, 'Live', $page1->ID, false) Versioned::get_versionnumber_by_stage(SiteTree::class, 'Live', $page1->ID, false)
)); ]);
$key = RateLimitFilter::CACHE_PREFIX . '_' . md5($key); $key = RateLimitFilter::CACHE_PREFIX . '_' . md5($key);
$cache->set($key, time() + 10); $this->cache->set($key, time() + 10);
$response = $this->get($page1->RelativeLink('changes')); $response = $this->get($page1->RelativeLink('changes'));
$this->assertEquals(429, $response->getStatusCode()); $this->assertEquals(429, $response->getStatusCode());
$this->assertGreaterThan(0, $response->getHeader('Retry-After')); $this->assertGreaterThan(0, $response->getHeader('Retry-After'));
@ -144,27 +153,22 @@ class VersionFeedFunctionalTest extends FunctionalTest
// Test rate limit hit by IP // Test rate limit hit by IP
Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', true); Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', true);
$_SERVER['REMOTE_ADDR'] = '127.0.0.1'; $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
$cache->set(RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1'), time() + 10); $this->cache->set(RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1'), time() + 10);
$response = $this->get($page1->RelativeLink('changes')); $response = $this->get($page1->RelativeLink('changes'));
$this->assertEquals(429, $response->getStatusCode()); $this->assertEquals(429, $response->getStatusCode());
$this->assertGreaterThan(0, $response->getHeader('Retry-After')); $this->assertGreaterThan(0, $response->getHeader('Retry-After'));
// Test rate limit doesn't hit other IP // Test rate limit doesn't hit other IP
$_SERVER['REMOTE_ADDR'] = '127.0.0.20'; $_SERVER['REMOTE_ADDR'] = '127.0.0.20';
$cache->set(RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1'), time() + 10); $this->cache->set(RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1'), time() + 10);
$response = $this->get($page1->RelativeLink('changes')); $response = $this->get($page1->RelativeLink('changes'));
$this->assertEquals(200, $response->getStatusCode()); $this->assertEquals(200, $response->getStatusCode());
// Restore setting
Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', false);
Config::modify()->set(RateLimitFilter::class, 'lock_timeout', 0);
Config::modify()->set(CachedContentFilter::class, 'cache_enabled', false);
} }
public function testChangesActionContainsChangesForCurrentPageOnly() public function testChangesActionContainsChangesForCurrentPageOnly()
{ {
$page1 = $this->createPageWithChanges(array('Title' => 'Page1')); $page1 = $this->createPageWithChanges(['Title' => 'Page1']);
$page2 = $this->createPageWithChanges(array('Title' => 'Page2')); $page2 = $this->createPageWithChanges(['Title' => 'Page2']);
$response = $this->get($page1->RelativeLink('changes')); $response = $this->get($page1->RelativeLink('changes'));
$xml = simplexml_load_string($response->getBody()); $xml = simplexml_load_string($response->getBody());
@ -187,8 +191,8 @@ class VersionFeedFunctionalTest extends FunctionalTest
public function testAllChangesActionContainsAllChangesForAllPages() public function testAllChangesActionContainsAllChangesForAllPages()
{ {
$page1 = $this->createPageWithChanges(array('Title' => 'Page1')); $page1 = $this->createPageWithChanges(['Title' => 'Page1']);
$page2 = $this->createPageWithChanges(array('Title' => 'Page2')); $page2 = $this->createPageWithChanges(['Title' => 'Page2']);
$response = $this->get($page1->RelativeLink('allchanges')); $response = $this->get($page1->RelativeLink('allchanges'));
$xml = simplexml_load_string($response->getBody()); $xml = simplexml_load_string($response->getBody());
@ -203,32 +207,32 @@ class VersionFeedFunctionalTest extends FunctionalTest
{ {
$page = new Page(); $page = new Page();
$seed = array_merge(array( $seed = array_merge([
'Title' => 'My Title', 'Title' => 'My Title',
'Content' => 'My Content' 'Content' => 'My Content'
), $seed); ], $seed);
$page->update($seed); $page->update($seed);
$page->write(); $page->write();
$page->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE); $page->publishSingle();
$page->update(array( $page->update([
'Title' => 'Changed: ' . $seed['Title'], 'Title' => 'Changed: ' . $seed['Title'],
'Content' => 'Changed: ' . $seed['Content'], 'Content' => 'Changed: ' . $seed['Content'],
)); ]);
$page->write(); $page->write();
$page->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE); $page->publishSingle();
$page->update(array( $page->update([
'Title' => 'Changed again: ' . $seed['Title'], 'Title' => 'Changed again: ' . $seed['Title'],
'Content' => 'Changed again: ' . $seed['Content'], 'Content' => 'Changed again: ' . $seed['Content'],
)); ]);
$page->write(); $page->write();
$page->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE); $page->publishSingle();
$page->update(array( $page->update([
'Title' => 'Unpublished: ' . $seed['Title'], 'Title' => 'Unpublished: ' . $seed['Title'],
'Content' => 'Unpublished: ' . $seed['Content'], 'Content' => 'Unpublished: ' . $seed['Content'],
)); ]);
$page->write(); $page->write();
return $page; return $page;
@ -241,11 +245,11 @@ class VersionFeedFunctionalTest extends FunctionalTest
{ {
// Nested loop through each configuration // Nested loop through each configuration
foreach (array(true, false) as $publicHistory_Page) { foreach ([true, false] as $publicHistory_Page) {
$page = $this->createPageWithChanges(array('PublicHistory' => $publicHistory_Page, 'Title' => 'Page')); $page = $this->createPageWithChanges(['PublicHistory' => $publicHistory_Page, 'Title' => 'Page']);
// Test requests to 'changes' action // Test requests to 'changes' action
foreach (array(true, false) as $publicHistory_Config) { foreach ([true, false] as $publicHistory_Config) {
Config::modify()->set(VersionFeed::class, 'changes_enabled', $publicHistory_Config); Config::modify()->set(VersionFeed::class, 'changes_enabled', $publicHistory_Config);
$expectedResponse = $publicHistory_Page && $publicHistory_Config ? 200 : 404; $expectedResponse = $publicHistory_Page && $publicHistory_Config ? 200 : 404;
$response = $this->get($page->RelativeLink('changes')); $response = $this->get($page->RelativeLink('changes'));
@ -253,8 +257,8 @@ class VersionFeedFunctionalTest extends FunctionalTest
} }
// Test requests to 'allchanges' action on each page // Test requests to 'allchanges' action on each page
foreach (array(true, false) as $allChanges_Config) { foreach ([true, false] as $allChanges_Config) {
foreach (array(true, false) as $allChanges_SiteConfig) { foreach ([true, false] as $allChanges_SiteConfig) {
Config::modify()->set(VersionFeed::class, 'allchanges_enabled', $allChanges_Config); Config::modify()->set(VersionFeed::class, 'allchanges_enabled', $allChanges_Config);
$siteConfig = SiteConfig::current_site_config(); $siteConfig = SiteConfig::current_site_config();
$siteConfig->AllChangesEnabled = $allChanges_SiteConfig; $siteConfig->AllChangesEnabled = $allChanges_SiteConfig;