Merge pull request #2983 from tractorcow/pulls/3.0-fix-versioned-partialcache

BUG Fix issue with versioned dataobjects being cached between stages

docs/en/changelogs/3.0.10.md

@@ -0,0 +1,12 @@
+# 3.0.10
+
+## Overview
+
+ * Security: Partially cached content from stage or other reading modes is no longer emitted to live
+
+## Upgrading
+
+ * If relying on partial caching of content between logged in users, be aware that the cache is now automatically
+   segmented based on both the current member ID, and the versioned reading mode. If this is not an appropriate
+   method (such as if the same content is served to logged in users within partial caching) then it is necessary
+   to adjust the config value of `SSViewer::global_key` to something more or less sensitive.

docs/en/reference/partial-caching.md

@@ -51,6 +51,19 @@ From a block that shows a summary of the page edits if administrator, nothing if
 	<% cached 'loginblock', LastEdited, CurrentMember.isAdmin %>
 
 
+An additional global key is incorporated in the cache lookup. The default value for this is
+`$CurrentReadingMode, $CurrentUser.ID`, which ensures that the current `[api:Versioned]` state and user ID are
+used. This may be configured by changing the config value of `SSViewer.global_key`. It is also necessary
+to flush the template caching when modifying this config, as this key is cached within the template itself.
+
+For example, to ensure that the cache is configured to respect another variable, and if the current logged in
+user does not influence your template content, you can update this key as below;
+
+	:::yaml
+	SSViewer:
+		global_key: '$CurrentReadingMode, $Locale'
+	
+
 ## Aggregates
 
 Often you want to invalidate a cache when any in a set of objects change, or when the objects in a relationship change.

model/Versioned.php

@@ -5,7 +5,8 @@
  * @package framework
  * @subpackage model
  */
-class Versioned extends DataExtension {
+class Versioned extends DataExtension implements TemplateGlobalProvider {
+	
 	/**
 	 * An array of possible stages.
 	 * @var array
@@ -1144,6 +1145,13 @@ class Versioned extends DataExtension {
 	public function cacheKeyComponent() {
 		return 'versionedmode-'.self::get_reading_mode();
 	}
+
+	public static function get_template_global_variables() {
+		return array(
+			'CurrentReadingMode' => 'get_reading_mode'
+		);
+	}
+
 }
 
 /**

tests/view/SSViewerCacheBlockTest.php

@@ -20,9 +20,29 @@ class SSViewerCacheBlockTest_Model extends DataObject implements TestOnly {
 	}
 }
 
+class SSViewerCacheBlockTest_VersionedModel extends DataObject implements TestOnly {
+	
+	protected $entropy = 'default';
+	
+	public static $extensions = array(
+		"Versioned('Stage', 'Live')"
+	);
+	
+	public function setEntropy($entropy) {
+		$this->entropy = $entropy;
+	}
+	
+	public function Inspect() {
+		return $this->entropy . ' ' . Versioned::get_reading_mode();
+	}
+}
+
 class SSViewerCacheBlockTest extends SapphireTest {
 	
-	protected $extraDataObjects = array('SSViewerCacheBlockTest_Model');
+	protected $extraDataObjects = array(
+		'SSViewerCacheBlockTest_Model',
+		'SSViewerCacheBlockTest_VersionedModel'
+	);
 	
 	protected $data = null;
 	
@@ -37,8 +57,7 @@ class SSViewerCacheBlockTest extends SapphireTest {
 		if ($data === null) $data = $this->data;
 		if (is_array($data)) $data = $this->data->customise($data);
 		
-		$viewer = SSViewer::fromString($template);
+		return SSViewer::execute_string($template, $data);
-		return $viewer->process($data);
 	}
 	
 	public function testParsing() {
@@ -105,6 +124,77 @@ class SSViewerCacheBlockTest extends SapphireTest {
 		$this->assertEquals($this->_runtemplate('<% cached %>$Foo<% end_cached %>', array('Foo' => 2)), '1');
 	}
 	
+	public function testVersionedCache() {
+				
+		$origStage = Versioned::current_stage();
+		
+		// Run without caching in stage to prove data is uncached
+		$this->_reset(false);
+		Versioned::reading_stage("Stage");
+		$data = new SSViewerCacheBlockTest_VersionedModel();
+		$data->setEntropy('default');
+		$this->assertEquals(
+			'default Stage.Stage',
+			SSViewer::execute_string('<% cached %>$Inspect<% end_cached %>', $data)
+		);
+		$data = new SSViewerCacheBlockTest_VersionedModel();
+		$data->setEntropy('first');
+		$this->assertEquals(
+			'first Stage.Stage',
+			SSViewer::execute_string('<% cached %>$Inspect<% end_cached %>', $data)
+		);
+		
+		// Run without caching in live to prove data is uncached
+		$this->_reset(false);
+		Versioned::reading_stage("Live");
+		$data = new SSViewerCacheBlockTest_VersionedModel();
+		$data->setEntropy('default');
+		$this->assertEquals(
+			'default Stage.Live',
+			$this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data)
+		);
+		$data = new SSViewerCacheBlockTest_VersionedModel();
+		$data->setEntropy('first');
+		$this->assertEquals(
+			'first Stage.Live',
+			$this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data)
+		);
+		
+		// Then with caching, initially in draft, and then in live, to prove that
+		// changing the versioned reading mode doesn't cache between modes, but it does
+		// within them
+		$this->_reset(true);
+		Versioned::reading_stage("Stage");
+		$data = new SSViewerCacheBlockTest_VersionedModel();
+		$data->setEntropy('default');
+		$this->assertEquals(
+			'default Stage.Stage',
+			$this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data)
+		);
+		$data = new SSViewerCacheBlockTest_VersionedModel();
+		$data->setEntropy('first');
+		$this->assertEquals(
+			'default Stage.Stage', // entropy should be ignored due to caching
+			$this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data)
+		);
+		
+		Versioned::reading_stage('Live');
+		$data = new SSViewerCacheBlockTest_VersionedModel();
+		$data->setEntropy('first');
+		$this->assertEquals(
+			'first Stage.Live', // First hit in live, so display current entropy
+			$this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data)
+		);
+		$data = new SSViewerCacheBlockTest_VersionedModel();
+		$data->setEntropy('second');
+		$this->assertEquals(
+			'first Stage.Live', // entropy should be ignored due to caching
+			$this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data)
+		);
+		
+		Versioned::reading_stage($origStage);
+	}
+
 	/**
 	 * Test that cacheblocks conditionally cache with if
 	 */

tests/view/SSViewerTest.php

@@ -35,9 +35,8 @@ class SSViewerTest extends SapphireTest {
 	 * Small helper to render templates from strings
 	 */
 	public function render($templateString, $data = null) {
-		$t = SSViewer::fromString($templateString);
 		if(!$data) $data = new SSViewerTestFixture();
-		return $t->process($data);
+		return SSViewer::execute_string($templateString, $data);
 	}
 	
 	public function testRequirements() {

view/SSTemplateParser.php

@@ -646,7 +646,8 @@ class SSTemplateParser extends Parser {
 	}
 
 
-	/* Translate: "<%t" < Entity < (Default:QuotedString)? < (!("is" "=") < "is" < Context:QuotedString)? < (InjectionVariables)? > "%>" */
+	/* Translate: "<%t" < Entity < (Default:QuotedString)? < (!("is" "=") < "is" < Context:QuotedString)? <
+	(InjectionVariables)? > "%>" */
 	protected $match_Translate_typestack = array('Translate');
 	function match_Translate ($stack = array()) {
 		$matchrule = "Translate"; $result = $this->construct($matchrule, $matchrule, null);
@@ -2743,10 +2744,25 @@ class SSTemplateParser extends Parser {
 	function CacheBlock_CacheBlockTemplate(&$res, $sub){
 		// Get the block counter
 		$block = ++$res['subblocks'];
-		// Build the key for this block from the passed cache key, the block index, and the sha hash of the template
+		// Build the key for this block from the global key (evaluated in a closure within the template),
-		// itself
+		// the passed cache key, the block index, and the sha hash of the template.
-		$key = "'" . sha1($sub['php']) . (isset($res['key']) && $res['key'] ? "_'.sha1(".$res['key'].")" : "'") . 
+		$res['php'] .= '$keyExpression = function() use ($scope, $cache) {' . PHP_EOL;
-			".'_$block'";
+		$res['php'] .= '$val = \'\';' . PHP_EOL;
+		if($globalKey = Config::inst()->get('SSViewer', 'global_key')) {
+			// Embed the code necessary to evaluate the globalKey directly into the template,
+			// so that SSTemplateParser only needs to be called during template regeneration.
+			// Warning: If the global key is changed, it's necessary to flush the template cache.
+			$parser = new SSTemplateParser($globalKey);
+			$result = $parser->match_Template();
+			if(!$result) throw new SSTemplateParseException('Unexpected problem parsing template', $parser);
+			$res['php'] .= $result['php'] . PHP_EOL;
+		}
+		$res['php'] .= 'return $val;' . PHP_EOL;
+		$res['php'] .= '};' . PHP_EOL;
+		$key = 'sha1($keyExpression())' // Global key
+			. '.\'_' . sha1($sub['php']) // sha of template
+			. (isset($res['key']) && $res['key'] ? "_'.sha1(".$res['key'].")" : "'") // Passed key
+			. ".'_$block'"; // block index
 		// Get any condition
 		$condition = isset($res['condition']) ? $res['condition'] : '';
 		
@@ -3642,7 +3658,8 @@ class SSTemplateParser extends Parser {
 		$method = 'OpenBlock_Handle_'.$blockname;
 		if (method_exists($this, $method)) $res['php'] = $this->$method($res);
 		else {
-			throw new SSTemplateParseException('Unknown open block "'.$blockname.'" encountered. Perhaps you missed the closing tag or have mis-spelled it?', $this);
+			throw new SSTemplateParseException('Unknown open block "'.$blockname.'" encountered. Perhaps you missed ' .
+				' the closing tag or have mis-spelled it?', $this);
 		}
 	}
 
@@ -3711,7 +3728,8 @@ class SSTemplateParser extends Parser {
 
 	function MismatchedEndBlock__finalise(&$res) {
 		$blockname = $res['Word']['text'];
-		throw new SSTemplateParseException('Unexpected close tag end_'.$blockname.' encountered. Perhaps you have mis-nested blocks, or have mis-spelled a tag?', $this);
+		throw new SSTemplateParseException('Unexpected close tag end_' . $blockname . 
+			' encountered. Perhaps you have mis-nested blocks, or have mis-spelled a tag?', $this);
 	}
 
 	/* MalformedOpenTag: '<%' < !NotBlockTag Tag:Word  !( ( [ :BlockArguments ] )? > '%>' ) */
@@ -3796,7 +3814,8 @@ class SSTemplateParser extends Parser {
 
 	function MalformedOpenTag__finalise(&$res) {
 		$tag = $res['Tag']['text'];
-		throw new SSTemplateParseException("Malformed opening block tag $tag. Perhaps you have tried to use operators?", $this);
+		throw new SSTemplateParseException("Malformed opening block tag $tag. Perhaps you have tried to use operators?"
+			, $this);
 	}
 	
 	/* MalformedCloseTag: '<%' < Tag:('end_' :Word ) !( > '%>' ) */
@@ -3860,7 +3879,8 @@ class SSTemplateParser extends Parser {
 
 	function MalformedCloseTag__finalise(&$res) {
 		$tag = $res['Tag']['text'];
-		throw new SSTemplateParseException("Malformed closing block tag $tag. Perhaps you have tried to pass an argument to one?", $this);
+		throw new SSTemplateParseException("Malformed closing block tag $tag. Perhaps you have tried to pass an " .
+			"argument to one?", $this);
 	}
 	
 	/* MalformedBlock: MalformedOpenTag | MalformedCloseTag */
@@ -4448,10 +4468,12 @@ class SSTemplateParser extends Parser {
 		$text = stripslashes($text);
 		$text = addcslashes($text, '\'\\');
 
-		// TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this non-dynamically calculated
+		// TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this
+		// non-dynamically calculated
 		$text = preg_replace(
 			'/href\s*\=\s*\"\#/', 
-			'href="\' . (SSViewer::$options[\'rewriteHashlinks\'] ? strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . \'#',
+			'href="\' . (SSViewer::$options[\'rewriteHashlinks\'] ? strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . 
+				\'#',
 			$text
 		);
 
@@ -4467,10 +4489,10 @@ class SSTemplateParser extends Parser {
 	 * 
 	 * @static
 	 * @throws SSTemplateParseException
-	 * @param  $string - The source of the template
+	 * @param  $string The source of the template
-	 * @param string $templateName - The name of the template, normally the filename the template source was loaded from
+	 * @param string $templateName The name of the template, normally the filename the template source was loaded from
-	 * @param bool $includeDebuggingComments - True is debugging comments should be included in the output
+	 * @param bool $includeDebuggingComments True is debugging comments should be included in the output
-	 * @return mixed|string - The php that, when executed (via include or exec) will behave as per the template source
+	 * @return mixed|string The php that, when executed (via include or exec) will behave as per the template source
 	 */
 	static function compileString($string, $templateName = "", $includeDebuggingComments=false) {
 		if (!trim($string)) {
@@ -4481,7 +4503,8 @@ class SSTemplateParser extends Parser {
 			$parser = new SSTemplateParser($string);
 			$parser->includeDebuggingComments = $includeDebuggingComments;
 	
-			// Ignore UTF8 BOM at begining of string. TODO: Confirm this is needed, make sure SSViewer handles UTF (and other encodings) properly
+			// Ignore UTF8 BOM at begining of string. TODO: Confirm this is needed, make sure SSViewer handles UTF
+			// (and other encodings) properly
 			if(substr($string, 0,3) == pack("CCC", 0xef, 0xbb, 0xbf)) $parser->pos = 3;
 			
 			// Match the source against the parser
@@ -4494,12 +4517,14 @@ class SSTemplateParser extends Parser {
 		
 		// Include top level debugging comments if desired
 		if($includeDebuggingComments && $templateName && stripos($code, "<?xml") === false) {
-			// If this template is a full HTML page, then put the comments just inside the HTML tag to prevent any IE glitches
+			// If this template is a full HTML page, then put the comments just inside the HTML tag to prevent any IE 
+			// glitches
 			if(stripos($code, "<html") !== false) {
 				$code = preg_replace('/(<html[^>]*>)/i', "\\1<!-- template $templateName -->", $code);
 				$code = preg_replace('/(<\/html[^>]*>)/i', "<!-- end template $templateName -->\\1", $code);
 			} else {
-				$code = str_replace('<?php' . PHP_EOL, '<?php' . PHP_EOL . '$val .= \'<!-- template ' . $templateName . ' -->\';' . "\n", $code);
+				$code = str_replace('<?php' . PHP_EOL, '<?php' . PHP_EOL . '$val .= \'<!-- template ' . $templateName .
+					' -->\';' . "\n", $code);
 				$code .= "\n" . '$val .= \'<!-- end template ' . $templateName . ' -->\';';
 			}
 		}	

view/SSTemplateParser.php.inc

@@ -559,10 +559,25 @@ class SSTemplateParser extends Parser {
 	function CacheBlock_CacheBlockTemplate(&$res, $sub){
 		// Get the block counter
 		$block = ++$res['subblocks'];
-		// Build the key for this block from the passed cache key, the block index, and the sha hash of the template
+		// Build the key for this block from the global key (evaluated in a closure within the template),
-		// itself
+		// the passed cache key, the block index, and the sha hash of the template.
-		$key = "'" . sha1($sub['php']) . (isset($res['key']) && $res['key'] ? "_'.sha1(".$res['key'].")" : "'") . 
+		$res['php'] .= '$keyExpression = function() use ($scope, $cache) {' . PHP_EOL;
-			".'_$block'";
+		$res['php'] .= '$val = \'\';' . PHP_EOL;
+		if($globalKey = Config::inst()->get('SSViewer', 'global_key')) {
+			// Embed the code necessary to evaluate the globalKey directly into the template,
+			// so that SSTemplateParser only needs to be called during template regeneration.
+			// Warning: If the global key is changed, it's necessary to flush the template cache.
+			$parser = new SSTemplateParser($globalKey);
+			$result = $parser->match_Template();
+			if(!$result) throw new SSTemplateParseException('Unexpected problem parsing template', $parser);
+			$res['php'] .= $result['php'] . PHP_EOL;
+		}
+		$res['php'] .= 'return $val;' . PHP_EOL;
+		$res['php'] .= '};' . PHP_EOL;
+		$key = 'sha1($keyExpression())' // Global key
+			. '.\'_' . sha1($sub['php']) // sha of template
+			. (isset($res['key']) && $res['key'] ? "_'.sha1(".$res['key'].")" : "'") // Passed key
+			. ".'_$block'"; // block index
 		// Get any condition
 		$condition = isset($res['condition']) ? $res['condition'] : '';
 		

view/SSViewer.php

@@ -46,7 +46,6 @@ class SSViewer_Scope {
 	
 	private $localIndex;
 
-
 	public function __construct($item){
 		$this->item = $item;
 		$this->localIndex=0;
@@ -568,6 +567,14 @@ class SSViewer {
 	 */
 	protected $includeRequirements = true;
 
+	/**
+	 * Default prepended cache key for partial caching
+	 * 
+	 * @var string
+	 * @config
+	 */
+	static $global_key = '$CurrentReadingMode, $CurrentUser.ID';
+
 	/**
 	 * Create a template from a string instead of a .ss file
 	 * 
@@ -936,6 +943,11 @@ class SSViewer {
 	/**
 	 * Execute the given template, passing it the given data.
 	 * Used by the <% include %> template tag to process templates.
+	 * 
+	 * @param string $template Template name
+	 * @param mixed $data Data context
+	 * @param array $arguments Additional arguments
+	 * @return string Evaluated result
 	 */
 	public static function execute_template($template, $data, $arguments = null) {
 		$v = new SSViewer($template);
@@ -944,6 +956,23 @@ class SSViewer {
 		return $v->process($data, $arguments);
 	}
 	
+	/**
+	 * Execute the evaluated string, passing it the given data.
+	 * Used by partial caching to evaluate custom cache keys expressed using
+	 * template expressions
+	 * 
+	 * @param string $content Input string
+	 * @param mixed $data Data context
+	 * @param array $arguments Additional arguments
+	 * @return string Evaluated result
+	 */
+	public static function execute_string($content, $data, $arguments = null) {
+		$v = SSViewer::fromString($content);
+		$v->includeRequirements(false);
+		
+		return $v->process($data, $arguments);
+	}
+
 	public static function parseTemplateContent($content, $template="") {
 		return SSTemplateParser::compileString($content, $template, Director::isDev() && self::$source_file_comments);
 	}